Skip to content

Commit

Permalink
Do not consider E999 (syntax error) as a rule
Browse files Browse the repository at this point in the history
  • Loading branch information
dhruvmanila committed Jun 26, 2024
1 parent 1a2f044 commit 955fdb7
Show file tree
Hide file tree
Showing 51 changed files with 794 additions and 367 deletions.
30 changes: 15 additions & 15 deletions crates/ruff/src/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use tempfile::NamedTempFile;

use ruff_cache::{CacheKey, CacheKeyHasher};
use ruff_diagnostics::{DiagnosticKind, Fix};
use ruff_linter::message::Message;
use ruff_linter::message::{DiagnosticMessage, Message};
use ruff_linter::{warn_user, VERSION};
use ruff_macros::CacheKey;
use ruff_notebook::NotebookIndex;
Expand Down Expand Up @@ -333,12 +333,14 @@ impl FileCache {
let file = SourceFileBuilder::new(path.to_string_lossy(), &*lint.source).finish();
lint.messages
.iter()
.map(|msg| Message {
kind: msg.kind.clone(),
range: msg.range,
fix: msg.fix.clone(),
file: file.clone(),
noqa_offset: msg.noqa_offset,
.map(|msg| {
Message::Diagnostic(DiagnosticMessage {
kind: msg.kind.clone(),
range: msg.range,
fix: msg.fix.clone(),
file: file.clone(),
noqa_offset: msg.noqa_offset,
})
})
.collect()
};
Expand Down Expand Up @@ -412,18 +414,19 @@ impl LintCacheData {
notebook_index: Option<NotebookIndex>,
) -> Self {
let source = if let Some(msg) = messages.first() {
msg.file.source_text().to_owned()
msg.source_file().source_text().to_owned()
} else {
String::new() // No messages, no need to keep the source!
};

let messages = messages
.iter()
.filter_map(|message| message.as_diagnostic_message())
.map(|msg| {
// Make sure that all message use the same source file.
assert_eq!(
msg.file,
messages.first().unwrap().file,
&msg.file,
messages.first().unwrap().source_file(),
"message uses a different source file"
);
CacheMessage {
Expand Down Expand Up @@ -571,6 +574,7 @@ mod tests {
use test_case::test_case;

use ruff_cache::CACHE_DIR_NAME;
use ruff_linter::message::Message;
use ruff_linter::settings::flags;
use ruff_linter::settings::types::UnsafeFixes;
use ruff_python_ast::PySourceType;
Expand Down Expand Up @@ -633,11 +637,7 @@ mod tests {
UnsafeFixes::Enabled,
)
.unwrap();
if diagnostics
.messages
.iter()
.any(|m| m.kind.name == "SyntaxError")
{
if diagnostics.messages.iter().any(Message::is_syntax_error) {
parse_errors.push(path.clone());
}
paths.push(path);
Expand Down
84 changes: 45 additions & 39 deletions crates/ruff/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@ use rustc_hash::FxHashMap;
use ruff_diagnostics::Diagnostic;
use ruff_linter::linter::{lint_fix, lint_only, FixTable, FixerResult, LinterResult, ParseSource};
use ruff_linter::logging::DisplayParseError;
use ruff_linter::message::Message;
use ruff_linter::message::{Message, SyntaxErrorMessage};
use ruff_linter::pyproject_toml::lint_pyproject_toml;
use ruff_linter::registry::AsRule;
use ruff_linter::settings::types::UnsafeFixes;
use ruff_linter::settings::{flags, LinterSettings};
use ruff_linter::source_kind::{SourceError, SourceKind};
use ruff_linter::{fs, IOError, SyntaxError};
use ruff_linter::{fs, IOError};
use ruff_notebook::{Notebook, NotebookError, NotebookIndex};
use ruff_python_ast::{PySourceType, SourceType, TomlSourceType};
use ruff_source_file::SourceFileBuilder;
Expand Down Expand Up @@ -55,57 +55,63 @@ impl Diagnostics {
path: Option<&Path>,
settings: &LinterSettings,
) -> Self {
let diagnostic = match err {
match err {
// IO errors.
SourceError::Io(_)
| SourceError::Notebook(NotebookError::Io(_) | NotebookError::Json(_)) => {
Diagnostic::new(
let diagnostic = Diagnostic::new(
IOError {
message: err.to_string(),
},
TextRange::default(),
)
);

if settings.rules.enabled(diagnostic.kind.rule()) {
let name = path.map_or_else(|| "-".into(), Path::to_string_lossy);
let dummy = SourceFileBuilder::new(name, "").finish();
Self::new(
vec![Message::from_diagnostic(
diagnostic,
dummy,
TextSize::default(),
)],
FxHashMap::default(),
)
} else {
match path {
Some(path) => {
warn!(
"{}{}{} {err}",
"Failed to lint ".bold(),
fs::relativize_path(path).bold(),
":".bold()
);
}
None => {
warn!("{}{} {err}", "Failed to lint".bold(), ":".bold());
}
}

Self::default()
}
}
// Syntax errors.
SourceError::Notebook(
NotebookError::InvalidJson(_)
| NotebookError::InvalidSchema(_)
| NotebookError::InvalidFormat(_),
) => Diagnostic::new(
SyntaxError {
message: err.to_string(),
},
TextRange::default(),
),
};

if settings.rules.enabled(diagnostic.kind.rule()) {
let name = path.map_or_else(|| "-".into(), Path::to_string_lossy);
let dummy = SourceFileBuilder::new(name, "").finish();
Self::new(
vec![Message::from_diagnostic(
diagnostic,
dummy,
TextSize::default(),
)],
FxHashMap::default(),
)
} else {
match path {
Some(path) => {
warn!(
"{}{}{} {err}",
"Failed to lint ".bold(),
fs::relativize_path(path).bold(),
":".bold()
);
}
None => {
warn!("{}{} {err}", "Failed to lint".bold(), ":".bold());
}
) => {
let name = path.map_or_else(|| "-".into(), Path::to_string_lossy);
let dummy = SourceFileBuilder::new(name, "").finish();
Self::new(
vec![Message::SyntaxError(SyntaxErrorMessage {
message: err.to_string(),
range: TextRange::default(),
file: dummy,
})],
FxHashMap::default(),
)
}

Self::default()
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion crates/ruff/src/printer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,7 @@ impl Printer {
let statistics: Vec<ExpandedStatistics> = diagnostics
.messages
.iter()
.filter_map(|message| message.as_diagnostic_message())
.map(|message| (message.kind.rule(), message.fix.is_some()))
.sorted()
.fold(vec![], |mut acc, (rule, fixable)| {
Expand Down Expand Up @@ -543,7 +544,7 @@ impl FixableStatistics {
let mut unapplicable_unsafe = 0;

for message in &diagnostics.messages {
if let Some(fix) = &message.fix {
if let Some(fix) = message.fix() {
if fix.applies(unsafe_fixes.required_applicability()) {
applicable += 1;
} else {
Expand Down
25 changes: 4 additions & 21 deletions crates/ruff/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -731,7 +731,7 @@ fn stdin_parse_error() {
success: false
exit_code: 1
----- stdout -----
-:1:16: E999 SyntaxError: Expected one or more symbol names after import
-:1:16: SyntaxError: Expected one or more symbol names after import
Found 1 error.
----- stderr -----
Expand All @@ -747,8 +747,8 @@ fn stdin_multiple_parse_error() {
success: false
exit_code: 1
----- stdout -----
-:1:16: E999 SyntaxError: Expected one or more symbol names after import
-:2:6: E999 SyntaxError: Expected an expression
-:1:16: SyntaxError: Expected one or more symbol names after import
-:2:6: SyntaxError: Expected an expression
Found 2 errors.
----- stderr -----
Expand All @@ -765,24 +765,7 @@ fn parse_error_not_included() {
success: false
exit_code: 1
----- stdout -----
-:1:6: E999 SyntaxError: Expected an expression
Found 1 error.
----- stderr -----
error: Failed to parse at 1:6: Expected an expression
"###);
}

#[test]
fn parse_error_ignored() {
// Explicitly ignore the `E999` rule
let mut cmd = RuffCheck::default().args(["--ignore=E999"]).build();
assert_cmd_snapshot!(cmd
.pass_stdin("foo =\n"), @r###"
success: false
exit_code: 1
----- stdout -----
-:1:6: E999 SyntaxError: Expected an expression
-:1:6: SyntaxError: Expected an expression
Found 1 error.
----- stderr -----
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ linter.rules.enabled = [
ambiguous-class-name (E742),
ambiguous-function-name (E743),
io-error (E902),
syntax-error (E999),
unused-import (F401),
import-shadowed-by-loop-var (F402),
undefined-local-with-import-star (F403),
Expand Down Expand Up @@ -148,7 +147,6 @@ linter.rules.should_fix = [
ambiguous-class-name (E742),
ambiguous-function-name (E743),
io-error (E902),
syntax-error (E999),
unused-import (F401),
import-shadowed-by-loop-var (F402),
undefined-local-with-import-star (F403),
Expand Down

This file was deleted.

1 change: 0 additions & 1 deletion crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,6 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pycodestyle, "E742") => (RuleGroup::Stable, rules::pycodestyle::rules::AmbiguousClassName),
(Pycodestyle, "E743") => (RuleGroup::Stable, rules::pycodestyle::rules::AmbiguousFunctionName),
(Pycodestyle, "E902") => (RuleGroup::Stable, rules::pycodestyle::rules::IOError),
(Pycodestyle, "E999") => (RuleGroup::Stable, rules::pycodestyle::rules::SyntaxError),

// pycodestyle warnings
(Pycodestyle, "W191") => (RuleGroup::Stable, rules::pycodestyle::rules::TabIndentation),
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pub use registry::clap_completion::RuleParser;
#[cfg(feature = "clap")]
pub use rule_selector::clap_completion::RuleSelectorParser;
pub use rule_selector::RuleSelector;
pub use rules::pycodestyle::rules::{IOError, SyntaxError};
pub use rules::pycodestyle::rules::IOError;

pub const VERSION: &str = env!("CARGO_PKG_VERSION");

Expand Down
24 changes: 10 additions & 14 deletions crates/ruff_linter/src/linter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ use crate::logging::DisplayParseError;
use crate::message::Message;
use crate::noqa::add_noqa;
use crate::registry::{AsRule, Rule, RuleSet};
use crate::rules::pycodestyle;
#[cfg(any(feature = "test-rules", test))]
use crate::rules::ruff::rules::test_rules::{self, TestRule, TEST_RULES};
use crate::settings::types::UnsafeFixes;
Expand Down Expand Up @@ -301,13 +300,6 @@ pub fn check_path(
}
}

for parse_error in parsed.errors() {
// Always add a diagnostic for the syntax error, regardless of whether `Rule::SyntaxError`
// is enabled. This should be done only after all the disablement methods have been
// processed like `per-file-ignores` and `noqa` comments.
pycodestyle::rules::syntax_error(&mut diagnostics, parse_error, locator);
}

// Remove fixes for any rules marked as unfixable.
for diagnostic in &mut diagnostics {
if !settings.rules.should_fix(diagnostic.kind.rule()) {
Expand Down Expand Up @@ -449,12 +441,15 @@ pub fn lint_only(
&parsed,
);

result.map(|diagnostics| diagnostics_to_messages(diagnostics, path, &locator, &directives))
result.map(|diagnostics| {
diagnostics_to_messages(diagnostics, parsed.errors(), path, &locator, &directives)
})
}

/// Convert from diagnostics to messages.
fn diagnostics_to_messages(
diagnostics: Vec<Diagnostic>,
parse_errors: &[ParseError],
path: &Path,
locator: &Locator,
directives: &Directives,
Expand All @@ -470,12 +465,13 @@ fn diagnostics_to_messages(
builder.finish()
});

diagnostics
.into_iter()
.map(|diagnostic| {
parse_errors
.iter()
.map(|parse_error| Message::from_parse_error(parse_error, locator, file.deref().clone()))
.chain(diagnostics.into_iter().map(|diagnostic| {
let noqa_offset = directives.noqa_line_for.resolve(diagnostic.start());
Message::from_diagnostic(diagnostic, file.deref().clone(), noqa_offset)
})
}))
.collect()
}

Expand Down Expand Up @@ -584,7 +580,7 @@ pub fn lint_fix<'a>(

return Ok(FixerResult {
result: result.map(|diagnostics| {
diagnostics_to_messages(diagnostics, path, &locator, &directives)
diagnostics_to_messages(diagnostics, parsed.errors(), path, &locator, &directives)
}),
transformed,
fixed,
Expand Down
Loading

0 comments on commit 955fdb7

Please sign in to comment.