Skip to content

Start detecting version-related syntax errors in the parser #16090

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 55 commits into from
Feb 26, 2025
Merged
Show file tree
Hide file tree
Changes from 53 commits
Commits
Show all changes
55 commits
Select commit Hold shift + click to select a range
e871194
add unused Parser::syntax_errors field
ntBre Feb 10, 2025
591063f
add hard-coded python version and detect `match`
ntBre Feb 10, 2025
e628e66
make PythonVersion more public, convert SyntaxError in red-knot
ntBre Feb 10, 2025
36ee9cd
process SyntaxErrors in ruff
ntBre Feb 10, 2025
611bda9
pass target version at the very end
ntBre Feb 10, 2025
bfb3bb5
pass tests
ntBre Feb 10, 2025
a6b8e34
add ruff test case
ntBre Feb 10, 2025
288821d
detect late future imports in the parser
ntBre Feb 10, 2025
ed67b01
pass f404 tests
ntBre Feb 10, 2025
fbf8765
check if rules are enabled before converting to diagnostics
ntBre Feb 10, 2025
5768f71
add a todo about duplicate diagnostics
ntBre Feb 10, 2025
2ecf3f2
clippy
ntBre Feb 10, 2025
ef858bf
update to use ast::PythonVersion and Span
ntBre Feb 14, 2025
d817289
remove LateFutureImport detection from the parser
ntBre Feb 15, 2025
6e4b956
tidy up
ntBre Feb 15, 2025
1805a65
clean up after rebase
ntBre Feb 19, 2025
cf77087
add ParseOptions::target_version
ntBre Feb 19, 2025
3c67b03
filter syntax errors when parsing, store version on SyntaxError
ntBre Feb 19, 2025
09ccf65
make ParseOptions Clone
ntBre Feb 19, 2025
a6bc0db
pass ParseOptions::target_version in the linter
ntBre Feb 19, 2025
49a48f2
pass ParseOptions::target_version in red-knot
ntBre Feb 19, 2025
79e6322
ignore version-related syntax errors when fuzzing
ntBre Feb 19, 2025
12d04b0
Red-knot no longer panics!
AlexWaygood Feb 21, 2025
efb9c2a
Fix fuzzing script and fix Rust formatting
AlexWaygood Feb 21, 2025
ee30486
Revert "ignore version-related syntax errors when fuzzing"
ntBre Feb 21, 2025
c8a22c5
pass check_file_skips_type_checking_when by initializing settings
ntBre Feb 21, 2025
380aaff
fix clippy lint about &impl ToString
ntBre Feb 21, 2025
80be7f7
pass version to new parsed_module call
ntBre Feb 21, 2025
6b69bb5
use LinterSettings::resolve_target_version
ntBre Feb 24, 2025
cd02f61
add ok test for match
ntBre Feb 24, 2025
cd90966
gate syntax error reporting behind preview
ntBre Feb 24, 2025
db77d9e
add docs for SyntaxError
ntBre Feb 24, 2025
1210a2f
use PythonVersion Display
ntBre Feb 24, 2025
8c4cf8c
impl Display instead of separate `message` method
ntBre Feb 24, 2025
a2f70cd
remove unused match
ntBre Feb 24, 2025
5a7906d
rename version to minimum_version
ntBre Feb 24, 2025
63e4a5a
mdtest version-related errors in red-knot
ntBre Feb 24, 2025
f3e5636
pass existing tests using `match`
ntBre Feb 24, 2025
3b93520
just use InvalidSyntax variant for version errors
ntBre Feb 25, 2025
16f1ae2
use SyntaxError::minimum_version
ntBre Feb 25, 2025
3dbb7f7
delete unused SyntaxErrorKind::as_str
ntBre Feb 25, 2025
be1a6fd
SyntaxError -> UnsupportedSyntaxError
ntBre Feb 25, 2025
1714b60
SyntaxErrorKind -> UnsupportedSyntaxErrorKind
ntBre Feb 25, 2025
418dd0f
rename fields and methods too
ntBre Feb 25, 2025
d78463c
move target_version out of loop
ntBre Feb 25, 2025
c672d22
only mark `match` for diagnostic
ntBre Feb 25, 2025
1a363ab
update is_valid docs
ntBre Feb 25, 2025
8035b13
update as_result and into_result docs too
ntBre Feb 25, 2025
bbb4bc4
always include unsupported_syntax_errors
ntBre Feb 25, 2025
18d9b6c
check for unsupported syntax errors in ruff_server
ntBre Feb 25, 2025
9980a6a
pass target_version to check_path
ntBre Feb 25, 2025
45c3b67
revert red-knot changes
ntBre Feb 25, 2025
8b14483
new -> added
ntBre Feb 25, 2025
76d507b
add checkpoint for unsupported_syntax_errors
ntBre Feb 25, 2025
5711091
chain diagnostics
ntBre Feb 26, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion crates/ruff/src/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -586,14 +586,15 @@ mod tests {
use anyhow::Result;
use filetime::{set_file_mtime, FileTime};
use itertools::Itertools;
use ruff_linter::settings::LinterSettings;
use test_case::test_case;

use ruff_cache::CACHE_DIR_NAME;
use ruff_linter::message::Message;
use ruff_linter::package::PackageRoot;
use ruff_linter::settings::flags;
use ruff_linter::settings::types::UnsafeFixes;
use ruff_python_ast::PySourceType;
use ruff_python_ast::{PySourceType, PythonVersion};
use ruff_workspace::Settings;

use crate::cache::{self, FileCache, FileCacheData, FileCacheKey};
Expand All @@ -611,6 +612,10 @@ mod tests {

let settings = Settings {
cache_dir,
linter: LinterSettings {
unresolved_target_version: PythonVersion::PY310,
..Default::default()
},
..Settings::default()
};

Expand Down
74 changes: 74 additions & 0 deletions crates/ruff/tests/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2627,3 +2627,77 @@ class A(Generic[T]):
"
);
}

#[test]
fn match_before_py310() {
// ok on 3.10
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.args(["--stdin-filename", "test.py"])
.arg("--target-version=py310")
.arg("-")
.pass_stdin(
r#"
match 2:
case 1:
print("it's one")
"#
),
@r"
success: true
exit_code: 0
----- stdout -----
All checks passed!

----- stderr -----
"
);

// ok on 3.9 without preview
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.args(["--stdin-filename", "test.py"])
.arg("--target-version=py39")
.arg("-")
.pass_stdin(
r#"
match 2:
case 1:
print("it's one")
"#
),
@r"
success: true
exit_code: 0
----- stdout -----
All checks passed!

----- stderr -----
"
);

// syntax error on 3.9 with preview
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.args(["--stdin-filename", "test.py"])
.arg("--target-version=py39")
.arg("--preview")
.arg("-")
.pass_stdin(
r#"
match 2:
case 1:
print("it's one")
"#
),
@r"
success: false
exit_code: 1
----- stdout -----
test.py:2:1: SyntaxError: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10)
Found 1 error.

----- stderr -----
"
);
}
66 changes: 54 additions & 12 deletions crates/ruff_linter/src/linter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ use rustc_hash::FxHashMap;

use ruff_diagnostics::Diagnostic;
use ruff_notebook::Notebook;
use ruff_python_ast::{ModModule, PySourceType};
use ruff_python_ast::{ModModule, PySourceType, PythonVersion};
use ruff_python_codegen::Stylist;
use ruff_python_index::Indexer;
use ruff_python_parser::{ParseError, Parsed};
use ruff_python_parser::{ParseError, ParseOptions, Parsed, UnsupportedSyntaxError};
use ruff_source_file::SourceFileBuilder;
use ruff_text_size::Ranged;

Expand Down Expand Up @@ -71,6 +71,7 @@ pub fn check_path(
source_kind: &SourceKind,
source_type: PySourceType,
parsed: &Parsed<ModModule>,
target_version: PythonVersion,
) -> Vec<Diagnostic> {
// Aggregate all diagnostics.
let mut diagnostics = vec![];
Expand Down Expand Up @@ -104,8 +105,6 @@ pub fn check_path(
));
}

let target_version = settings.resolve_target_version(path);

// Run the filesystem-based rules.
if settings
.rules
Expand Down Expand Up @@ -335,7 +334,8 @@ pub fn add_noqa_to_path(
settings: &LinterSettings,
) -> Result<usize> {
// Parse once.
let parsed = ruff_python_parser::parse_unchecked_source(source_kind.source_code(), source_type);
let target_version = settings.resolve_target_version(path);
let parsed = parse_unchecked_source(source_kind, source_type, target_version);

// Map row and column locations to byte slices (lazily).
let locator = Locator::new(source_kind.source_code());
Expand Down Expand Up @@ -367,6 +367,7 @@ pub fn add_noqa_to_path(
source_kind,
source_type,
&parsed,
target_version,
);

// Add any missing `# noqa` pragmas.
Expand All @@ -393,7 +394,8 @@ pub fn lint_only(
source_type: PySourceType,
source: ParseSource,
) -> LinterResult {
let parsed = source.into_parsed(source_kind, source_type);
let target_version = settings.resolve_target_version(path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect we're now resolving the target_version multiple times. Once here and then somewhere inside check_path? I guess that's probably fine but I wonder how much work it would be to avoid it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh good catch. I was able to remove the resolution in check_path by passing the version to it everywhere. The other three resolutions in the linter are in add_noqa_to_path, lint_only, and lint_fix and right before parse_unchecked calls, so I think those are needed.

This is probably a separate issue, but do we need to re-parse (and re-resolve) in add_noqa_to_path? I guess it gets called before we know what else we're doing with the files.

let parsed = source.into_parsed(source_kind, source_type, target_version);

// Map row and column locations to byte slices (lazily).
let locator = Locator::new(source_kind.source_code());
Expand Down Expand Up @@ -425,12 +427,20 @@ pub fn lint_only(
source_kind,
source_type,
&parsed,
target_version,
);

let syntax_errors = if settings.preview.is_enabled() {
parsed.unsupported_syntax_errors()
} else {
&[]
};

LinterResult {
messages: diagnostics_to_messages(
diagnostics,
parsed.errors(),
syntax_errors,
path,
&locator,
&directives,
Expand All @@ -443,6 +453,7 @@ pub fn lint_only(
fn diagnostics_to_messages(
diagnostics: Vec<Diagnostic>,
parse_errors: &[ParseError],
unsupported_syntax_errors: &[UnsupportedSyntaxError],
path: &Path,
locator: &Locator,
directives: &Directives,
Expand All @@ -461,6 +472,9 @@ fn diagnostics_to_messages(
parse_errors
.iter()
.map(|parse_error| Message::from_parse_error(parse_error, locator, file.deref().clone()))
.chain(unsupported_syntax_errors.iter().map(|syntax_error| {
Message::from_unsupported_syntax_error(syntax_error, 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)
Expand Down Expand Up @@ -491,11 +505,12 @@ pub fn lint_fix<'a>(
// Track whether the _initial_ source code is valid syntax.
let mut is_valid_syntax = false;

let target_version = settings.resolve_target_version(path);

// Continuously fix until the source code stabilizes.
loop {
// Parse once.
let parsed =
ruff_python_parser::parse_unchecked_source(transformed.source_code(), source_type);
let parsed = parse_unchecked_source(&transformed, source_type, target_version);

// Map row and column locations to byte slices (lazily).
let locator = Locator::new(transformed.source_code());
Expand Down Expand Up @@ -527,6 +542,7 @@ pub fn lint_fix<'a>(
&transformed,
source_type,
&parsed,
target_version,
);

if iterations == 0 {
Expand Down Expand Up @@ -573,11 +589,18 @@ pub fn lint_fix<'a>(
report_failed_to_converge_error(path, transformed.source_code(), &diagnostics);
}

let syntax_errors = if settings.preview.is_enabled() {
parsed.unsupported_syntax_errors()
} else {
&[]
};

return Ok(FixerResult {
result: LinterResult {
messages: diagnostics_to_messages(
diagnostics,
parsed.errors(),
syntax_errors,
path,
&locator,
&directives,
Expand Down Expand Up @@ -680,16 +703,35 @@ pub enum ParseSource {
impl ParseSource {
/// Consumes the [`ParseSource`] and returns the parsed [`Parsed`], parsing the source code if
/// necessary.
fn into_parsed(self, source_kind: &SourceKind, source_type: PySourceType) -> Parsed<ModModule> {
fn into_parsed(
self,
source_kind: &SourceKind,
source_type: PySourceType,
target_version: PythonVersion,
) -> Parsed<ModModule> {
match self {
ParseSource::None => {
ruff_python_parser::parse_unchecked_source(source_kind.source_code(), source_type)
}
ParseSource::None => parse_unchecked_source(source_kind, source_type, target_version),
ParseSource::Precomputed(parsed) => parsed,
}
}
}

/// Like [`ruff_python_parser::parse_unchecked_source`] but with an additional [`PythonVersion`]
/// argument.
fn parse_unchecked_source(
source_kind: &SourceKind,
source_type: PySourceType,
target_version: PythonVersion,
) -> Parsed<ModModule> {
let options = ParseOptions::from(source_type).with_target_version(target_version);
// SAFETY: Safe because `PySourceType` always parses to a `ModModule`. See
// `ruff_python_parser::parse_unchecked_source`. We use `parse_unchecked` (and thus
// have to unwrap) in order to pass the `PythonVersion` via `ParseOptions`.
ruff_python_parser::parse_unchecked(source_kind.source_code(), options)
.try_into_module()
.expect("PySourceType always parses into a module")
}

#[cfg(test)]
mod tests {
use std::path::Path;
Expand Down
14 changes: 13 additions & 1 deletion crates/ruff_linter/src/message/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ pub use pylint::PylintEmitter;
pub use rdjson::RdjsonEmitter;
use ruff_diagnostics::{Diagnostic, DiagnosticKind, Fix};
use ruff_notebook::NotebookIndex;
use ruff_python_parser::ParseError;
use ruff_python_parser::{ParseError, UnsupportedSyntaxError};
use ruff_source_file::{SourceFile, SourceLocation};
use ruff_text_size::{Ranged, TextLen, TextRange, TextSize};
pub use sarif::SarifEmitter;
Expand Down Expand Up @@ -121,6 +121,18 @@ impl Message {
})
}

/// Create a [`Message`] from the given [`UnsupportedSyntaxError`].
pub fn from_unsupported_syntax_error(
unsupported_syntax_error: &UnsupportedSyntaxError,
file: SourceFile,
) -> Message {
Message::SyntaxError(SyntaxErrorMessage {
message: format!("SyntaxError: {unsupported_syntax_error}"),
range: unsupported_syntax_error.range,
file,
})
}

pub const fn as_diagnostic_message(&self) -> Option<&DiagnosticMessage> {
match self {
Message::Diagnostic(m) => Some(m),
Expand Down
9 changes: 7 additions & 2 deletions crates/ruff_linter/src/rules/pyflakes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ mod tests {

use anyhow::Result;
use regex::Regex;
use ruff_python_parser::ParseOptions;
use rustc_hash::FxHashMap;
use test_case::test_case;

Expand Down Expand Up @@ -744,8 +745,11 @@ mod tests {
let source_type = PySourceType::default();
let source_kind = SourceKind::Python(contents.to_string());
let settings = LinterSettings::for_rules(Linter::Pyflakes.rules());
let parsed =
ruff_python_parser::parse_unchecked_source(source_kind.source_code(), source_type);
let options =
ParseOptions::from(source_type).with_target_version(settings.unresolved_target_version);
let parsed = ruff_python_parser::parse_unchecked(source_kind.source_code(), options)
.try_into_module()
.expect("PySourceType always parses into a module");
let locator = Locator::new(&contents);
let stylist = Stylist::from_tokens(parsed.tokens(), locator.contents());
let indexer = Indexer::from_tokens(parsed.tokens(), locator.contents());
Expand All @@ -767,6 +771,7 @@ mod tests {
&source_kind,
source_type,
&parsed,
settings.unresolved_target_version,
);
diagnostics.sort_by_key(Ranged::start);
let actual = diagnostics
Expand Down
14 changes: 11 additions & 3 deletions crates/ruff_linter/src/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use ruff_notebook::NotebookError;
use ruff_python_ast::PySourceType;
use ruff_python_codegen::Stylist;
use ruff_python_index::Indexer;
use ruff_python_parser::ParseError;
use ruff_python_parser::{ParseError, ParseOptions};
use ruff_python_trivia::textwrap::dedent;
use ruff_source_file::SourceFileBuilder;
use ruff_text_size::Ranged;
Expand Down Expand Up @@ -110,7 +110,11 @@ pub(crate) fn test_contents<'a>(
settings: &LinterSettings,
) -> (Vec<Message>, Cow<'a, SourceKind>) {
let source_type = PySourceType::from(path);
let parsed = ruff_python_parser::parse_unchecked_source(source_kind.source_code(), source_type);
let target_version = settings.resolve_target_version(path);
let options = ParseOptions::from(source_type).with_target_version(target_version);
let parsed = ruff_python_parser::parse_unchecked(source_kind.source_code(), options.clone())
.try_into_module()
.expect("PySourceType always parses into a module");
let locator = Locator::new(source_kind.source_code());
let stylist = Stylist::from_tokens(parsed.tokens(), locator.contents());
let indexer = Indexer::from_tokens(parsed.tokens(), locator.contents());
Expand All @@ -134,6 +138,7 @@ pub(crate) fn test_contents<'a>(
source_kind,
source_type,
&parsed,
target_version,
);

let source_has_errors = !parsed.is_valid();
Expand Down Expand Up @@ -174,7 +179,9 @@ pub(crate) fn test_contents<'a>(
transformed = Cow::Owned(transformed.updated(fixed_contents, &source_map));

let parsed =
ruff_python_parser::parse_unchecked_source(transformed.source_code(), source_type);
ruff_python_parser::parse_unchecked(transformed.source_code(), options.clone())
.try_into_module()
.expect("PySourceType always parses into a module");
let locator = Locator::new(transformed.source_code());
let stylist = Stylist::from_tokens(parsed.tokens(), locator.contents());
let indexer = Indexer::from_tokens(parsed.tokens(), locator.contents());
Expand All @@ -197,6 +204,7 @@ pub(crate) fn test_contents<'a>(
&transformed,
source_type,
&parsed,
target_version,
);

if !parsed.is_valid() && !source_has_errors {
Expand Down
Loading
Loading