Skip to content

Commit

Permalink
fix(linter): fix ignorePattern config for windows (#8214)
Browse files Browse the repository at this point in the history
Changes:

- Config `ignorePatterns` not as `Path`, because the create `ignore`
does only handle `/` and not Win-style `\`
- Passed full path to the config and do not use `canonicalize` because
it prefix the paths with `\\?\`,
  read more here: https://stackoverflow.com/a/41233992/7387397
- Updated and enabled tests for windows, some are still failing 

closes #8188

---------

Co-authored-by: Alexander Schlegel <[email protected]>
  • Loading branch information
Sysix and Alexander Schlegel authored Jan 2, 2025
1 parent 0e168b8 commit 2b14a6f
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 28 deletions.
51 changes: 34 additions & 17 deletions apps/oxlint/src/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use std::{
};

use ignore::gitignore::Gitignore;

use oxc_diagnostics::{DiagnosticService, GraphicalReportHandler};
use oxc_linter::{
loader::LINT_PARTIAL_LOADER_EXT, AllowWarnDeny, InvalidFilterKind, LintFilter, LintService,
Expand All @@ -21,6 +22,7 @@ use crate::{
walk::{Extensions, Walk},
};

#[derive(Debug)]
pub struct LintRunner {
options: LintCommand,
cwd: PathBuf,
Expand Down Expand Up @@ -114,13 +116,9 @@ impl Runner for LintRunner {
}

let mut oxlintrc = config_search_result.unwrap();
let oxlint_wd = oxlintrc.path.parent().unwrap_or(&self.cwd).to_path_buf();

let ignore_paths = oxlintrc
.ignore_patterns
.iter()
.map(|value| oxlintrc.path.parent().unwrap().join(value))
.collect::<Vec<_>>();
let paths = Walk::new(&paths, &ignore_options, &ignore_paths)
let paths = Walk::new(&oxlint_wd, &paths, &ignore_options, &oxlintrc.ignore_patterns)
.with_extensions(Extensions(extensions))
.paths();

Expand Down Expand Up @@ -258,7 +256,10 @@ impl LintRunner {
// when no file is found, the default configuration is returned
fn find_oxlint_config(cwd: &Path, config: Option<&PathBuf>) -> Result<Oxlintrc, CliRunResult> {
if let Some(config_path) = config {
return match Oxlintrc::from_file(config_path) {
let mut full_path = cwd.to_path_buf();
full_path.push(config_path);

return match Oxlintrc::from_file(&full_path) {
Ok(config) => Ok(config),
Err(diagnostic) => {
let handler = GraphicalReportHandler::new();
Expand All @@ -281,9 +282,9 @@ impl LintRunner {
}
}

#[cfg(all(test, not(target_os = "windows")))]
#[cfg(test)]
mod test {
use std::env;
use std::{env, path::MAIN_SEPARATOR_STR};

use super::LintRunner;
use crate::cli::{lint_command, CliRunResult, LintResult, Runner};
Expand All @@ -301,10 +302,19 @@ mod test {
fn test_with_cwd(cwd: &str, args: &[&str]) -> LintResult {
let mut new_args = vec!["--silent"];
new_args.extend(args);

let options = lint_command().run_inner(new_args.as_slice()).unwrap();

let mut current_cwd = env::current_dir().unwrap();
current_cwd.push(cwd);

let part_cwd = if MAIN_SEPARATOR_STR == "/" {
cwd.into()
} else {
#[expect(clippy::disallowed_methods)]
cwd.replace('/', MAIN_SEPARATOR_STR)
};

current_cwd.push(part_cwd);

match LintRunner::new(options).with_cwd(current_cwd).run() {
CliRunResult::LintResult(lint_result) => lint_result,
Expand Down Expand Up @@ -343,6 +353,8 @@ mod test {
assert_eq!(result.number_of_errors, 0);
}

// exclude because we are working with Glob, which only supports the current working directory
#[cfg(all(test, not(target_os = "windows")))]
#[test]
fn cwd() {
let args = &["debugger.js"];
Expand Down Expand Up @@ -371,6 +383,8 @@ mod test {
assert_eq!(result.number_of_errors, 0);
}

// ToDo: lints all files under windows
#[cfg(all(test, not(target_os = "windows")))]
#[test]
fn wrong_extension() {
let args = &["foo.asdf"];
Expand Down Expand Up @@ -679,12 +693,16 @@ mod test {
use std::fs;
let file = "fixtures/linter/fix.js";
let args = &["--fix", file];
let content = fs::read_to_string(file).unwrap();
let content_original = fs::read_to_string(file).unwrap();
#[expect(clippy::disallowed_methods)]
let content = content_original.replace("\r\n", "\n");
assert_eq!(&content, "debugger\n");

// Apply fix to the file.
let _ = test(args);
assert_eq!(fs::read_to_string(file).unwrap(), "\n");
#[expect(clippy::disallowed_methods)]
let new_content = fs::read_to_string(file).unwrap().replace("\r\n", "\n");
assert_eq!(new_content, "\n");

// File should not be modified if no fix is applied.
let modified_before = fs::metadata(file).unwrap().modified().unwrap();
Expand All @@ -693,7 +711,7 @@ mod test {
assert_eq!(modified_before, modified_after);

// Write the file back.
fs::write(file, content).unwrap();
fs::write(file, content_original).unwrap();
}

#[test]
Expand Down Expand Up @@ -779,11 +797,10 @@ mod test {

#[test]
fn test_config_ignore_patterns_directory() {
let result = test(&[
"-c",
"fixtures/config_ignore_patterns/ignore_directory/eslintrc.json",
let result = test_with_cwd(
"fixtures/config_ignore_patterns/ignore_directory",
]);
&["-c", "eslintrc.json"],
);
assert_eq!(result.number_of_files, 1);
}

Expand Down
12 changes: 6 additions & 6 deletions apps/oxlint/src/walk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,14 @@ impl ignore::ParallelVisitor for WalkCollector {
}
}
}

impl Walk {
/// Will not canonicalize paths.
/// # Panics
pub fn new(
current_path: &PathBuf,
paths: &[PathBuf],
options: &IgnoreOptions,
config_ignore_patterns: &[PathBuf],
config_ignore_patterns: &Vec<String>,
) -> Self {
assert!(!paths.is_empty(), "At least one path must be provided to Walk::new");

Expand All @@ -91,7 +91,7 @@ impl Walk {
if !options.no_ignore {
inner.add_custom_ignore_filename(&options.ignore_path);

let mut override_builder = OverrideBuilder::new(Path::new("/"));
let mut override_builder = OverrideBuilder::new(current_path);
if !options.ignore_pattern.is_empty() {
for pattern in &options.ignore_pattern {
// Meaning of ignore pattern is reversed
Expand All @@ -103,7 +103,7 @@ impl Walk {

if !config_ignore_patterns.is_empty() {
for pattern in config_ignore_patterns {
let pattern = format!("!{}", pattern.to_str().unwrap());
let pattern = format!("!{pattern}");
override_builder.add(&pattern).unwrap();
}
}
Expand Down Expand Up @@ -148,7 +148,7 @@ impl Walk {

#[cfg(test)]
mod test {
use std::{env, ffi::OsString};
use std::{env, ffi::OsString, path::PathBuf};

use super::{Extensions, Walk};
use crate::cli::IgnoreOptions;
Expand All @@ -164,7 +164,7 @@ mod test {
symlinks: false,
};

let mut paths = Walk::new(&fixtures, &ignore_options, &[])
let mut paths = Walk::new(&PathBuf::from("/"), &fixtures, &ignore_options, &vec![])
.with_extensions(Extensions(["js", "vue"].to_vec()))
.paths()
.into_iter()
Expand Down
5 changes: 1 addition & 4 deletions crates/oxc_linter/src/config/oxlintrc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,10 +127,7 @@ impl Oxlintrc {
OxcDiagnostic::error(format!("Failed to parse config with error {err:?}"))
})?;

// Get absolute path from `path`
let absolute_path = path.canonicalize().unwrap_or_else(|_| path.to_path_buf());

config.path = absolute_path;
config.path = path.to_path_buf();

Ok(config)
}
Expand Down
2 changes: 1 addition & 1 deletion justfile
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ alias c := conformance
alias f := fix
alias new-typescript-rule := new-ts-rule

# Make sure you have cargo-binstall installed.
# Make sure you have cargo-binstall and pnpm installed.
# You can download the pre-compiled binary from <https://github.com/cargo-bins/cargo-binstall#installation>
# or install via `cargo install cargo-binstall`
# Initialize the project by installing all the necessary tools.
Expand Down

0 comments on commit 2b14a6f

Please sign in to comment.