Document LinterResult::has_syntax_error and add Parsed::has_no_syntax_errors (#16443)

Summary
--

This is a follow up addressing the comments on #16425. As @dhruvmanila
pointed out, the naming is a bit tricky. I went with `has_no_errors` to
try to differentiate it from `is_valid`. It actually ends up negated in
most uses, so it would be more convenient to have `has_any_errors` or
`has_errors`, but I thought it would sound too much like the opposite of
`is_valid` in that case. I'm definitely open to suggestions here.

Test Plan
--

Existing tests.
This commit is contained in:
Brent Westbrook 2025-03-04 08:35:38 -05:00 committed by GitHub
parent a3ae76edc0
commit 37fbe58b13
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 50 additions and 26 deletions

View file

@ -102,7 +102,7 @@ mod tests {
let parsed = parsed_module(&db, file); let parsed = parsed_module(&db, file);
assert!(parsed.is_valid()); assert!(parsed.has_valid_syntax());
Ok(()) Ok(())
} }
@ -118,7 +118,7 @@ mod tests {
let parsed = parsed_module(&db, file); let parsed = parsed_module(&db, file);
assert!(parsed.is_valid()); assert!(parsed.has_valid_syntax());
Ok(()) Ok(())
} }
@ -134,7 +134,7 @@ mod tests {
let parsed = parsed_module(&db, virtual_file.file()); let parsed = parsed_module(&db, virtual_file.file());
assert!(parsed.is_valid()); assert!(parsed.has_valid_syntax());
Ok(()) Ok(())
} }
@ -150,7 +150,7 @@ mod tests {
let parsed = parsed_module(&db, virtual_file.file()); let parsed = parsed_module(&db, virtual_file.file());
assert!(parsed.is_valid()); assert!(parsed.has_valid_syntax());
Ok(()) Ok(())
} }
@ -181,6 +181,6 @@ else:
let parsed = parsed_module(&db, file); let parsed = parsed_module(&db, file);
assert!(parsed.is_valid()); assert!(parsed.has_valid_syntax());
} }
} }

View file

@ -42,6 +42,8 @@ pub struct LinterResult {
pub messages: Vec<Message>, pub messages: Vec<Message>,
/// A flag indicating the presence of syntax errors in the source file. /// A flag indicating the presence of syntax errors in the source file.
/// If `true`, at least one syntax error was detected in the source file. /// If `true`, at least one syntax error was detected in the source file.
///
/// This includes both [`ParseError`]s and [`UnsupportedSyntaxError`]s.
pub has_syntax_error: bool, pub has_syntax_error: bool,
} }
@ -133,7 +135,7 @@ pub fn check_path(
} }
// Run the AST-based rules only if there are no syntax errors. // Run the AST-based rules only if there are no syntax errors.
if parsed.is_valid() { if parsed.has_valid_syntax() {
let use_ast = settings let use_ast = settings
.rules .rules
.iter_enabled() .iter_enabled()
@ -283,7 +285,7 @@ pub fn check_path(
locator, locator,
comment_ranges, comment_ranges,
&directives.noqa_line_for, &directives.noqa_line_for,
parsed.is_valid(), parsed.has_valid_syntax(),
&per_file_ignores, &per_file_ignores,
settings, settings,
); );
@ -294,7 +296,7 @@ pub fn check_path(
} }
} }
if parsed.is_valid() { if parsed.has_valid_syntax() {
// Remove fixes for any rules marked as unfixable. // Remove fixes for any rules marked as unfixable.
for diagnostic in &mut diagnostics { for diagnostic in &mut diagnostics {
if !settings.rules.should_fix(diagnostic.kind.rule()) { if !settings.rules.should_fix(diagnostic.kind.rule()) {
@ -445,7 +447,7 @@ pub fn lint_only(
&locator, &locator,
&directives, &directives,
), ),
has_syntax_error: !parsed.is_valid() || !parsed.unsupported_syntax_errors().is_empty(), has_syntax_error: parsed.has_syntax_errors(),
} }
} }
@ -546,7 +548,7 @@ pub fn lint_fix<'a>(
); );
if iterations == 0 { if iterations == 0 {
is_valid_syntax = parsed.is_valid() && parsed.unsupported_syntax_errors().is_empty(); is_valid_syntax = parsed.has_no_syntax_errors();
} else { } else {
// If the source code was parseable on the first pass, but is no // If the source code was parseable on the first pass, but is no
// longer parseable on a subsequent pass, then we've introduced a // longer parseable on a subsequent pass, then we've introduced a

View file

@ -141,7 +141,7 @@ pub(crate) fn test_contents<'a>(
target_version, target_version,
); );
let source_has_errors = !parsed.is_valid(); let source_has_errors = parsed.has_invalid_syntax();
// Detect fixes that don't converge after multiple iterations. // Detect fixes that don't converge after multiple iterations.
let mut iterations = 0; let mut iterations = 0;
@ -207,7 +207,7 @@ pub(crate) fn test_contents<'a>(
target_version, target_version,
); );
if !parsed.is_valid() && !source_has_errors { if parsed.has_invalid_syntax() && !source_has_errors {
// Previous fix introduced a syntax error, abort // Previous fix introduced a syntax error, abort
let fixes = print_diagnostics(diagnostics, path, source_kind); let fixes = print_diagnostics(diagnostics, path, source_kind);
let syntax_errors = let syntax_errors =

View file

@ -342,21 +342,47 @@ impl<T> Parsed<T> {
self.errors self.errors
} }
/// Returns `true` if the parsed source code is valid i.e., it has no syntax errors. /// Returns `true` if the parsed source code is valid i.e., it has no [`ParseError`]s.
/// ///
/// Note that this does not include version-related /// Note that this does not include version-related [`UnsupportedSyntaxError`]s.
/// [`unsupported_syntax_errors`](Parsed::unsupported_syntax_errors). ///
pub fn is_valid(&self) -> bool { /// See [`Parsed::has_no_syntax_errors`] for a version that takes these into account.
pub fn has_valid_syntax(&self) -> bool {
self.errors.is_empty() self.errors.is_empty()
} }
/// Returns `true` if the parsed source code is invalid i.e., it has [`ParseError`]s.
///
/// Note that this does not include version-related [`UnsupportedSyntaxError`]s.
///
/// See [`Parsed::has_no_syntax_errors`] for a version that takes these into account.
pub fn has_invalid_syntax(&self) -> bool {
!self.has_valid_syntax()
}
/// Returns `true` if the parsed source code does not contain any [`ParseError`]s *or*
/// [`UnsupportedSyntaxError`]s.
///
/// See [`Parsed::has_valid_syntax`] for a version specific to [`ParseError`]s.
pub fn has_no_syntax_errors(&self) -> bool {
self.has_valid_syntax() && self.unsupported_syntax_errors.is_empty()
}
/// Returns `true` if the parsed source code contains any [`ParseError`]s *or*
/// [`UnsupportedSyntaxError`]s.
///
/// See [`Parsed::has_invalid_syntax`] for a version specific to [`ParseError`]s.
pub fn has_syntax_errors(&self) -> bool {
!self.has_no_syntax_errors()
}
/// Returns the [`Parsed`] output as a [`Result`], returning [`Ok`] if it has no syntax errors, /// Returns the [`Parsed`] output as a [`Result`], returning [`Ok`] if it has no syntax errors,
/// or [`Err`] containing the first [`ParseError`] encountered. /// or [`Err`] containing the first [`ParseError`] encountered.
/// ///
/// Note that any [`unsupported_syntax_errors`](Parsed::unsupported_syntax_errors) will not /// Note that any [`unsupported_syntax_errors`](Parsed::unsupported_syntax_errors) will not
/// cause [`Err`] to be returned. /// cause [`Err`] to be returned.
pub fn as_result(&self) -> Result<&Parsed<T>, &[ParseError]> { pub fn as_result(&self) -> Result<&Parsed<T>, &[ParseError]> {
if self.is_valid() { if self.has_valid_syntax() {
Ok(self) Ok(self)
} else { } else {
Err(&self.errors) Err(&self.errors)
@ -369,7 +395,7 @@ impl<T> Parsed<T> {
/// Note that any [`unsupported_syntax_errors`](Parsed::unsupported_syntax_errors) will not /// Note that any [`unsupported_syntax_errors`](Parsed::unsupported_syntax_errors) will not
/// cause [`Err`] to be returned. /// cause [`Err`] to be returned.
pub(crate) fn into_result(self) -> Result<Parsed<T>, ParseError> { pub(crate) fn into_result(self) -> Result<Parsed<T>, ParseError> {
if self.is_valid() { if self.has_valid_syntax() {
Ok(self) Ok(self)
} else { } else {
Err(self.into_errors().into_iter().next().unwrap()) Err(self.into_errors().into_iter().next().unwrap())

View file

@ -39,9 +39,7 @@ fn test_valid_syntax(input_path: &Path) {
}); });
let parsed = parse_unchecked(&source, options); let parsed = parse_unchecked(&source, options);
let is_valid = parsed.is_valid() && parsed.unsupported_syntax_errors().is_empty(); if parsed.has_syntax_errors() {
if !is_valid {
let line_index = LineIndex::from_source_text(&source); let line_index = LineIndex::from_source_text(&source);
let source_code = SourceCode::new(&source, &line_index); let source_code = SourceCode::new(&source, &line_index);
@ -101,10 +99,8 @@ fn test_invalid_syntax(input_path: &Path) {
}); });
let parsed = parse_unchecked(&source, options); let parsed = parse_unchecked(&source, options);
let is_valid = parsed.is_valid() && parsed.unsupported_syntax_errors().is_empty();
assert!( assert!(
!is_valid, parsed.has_syntax_errors(),
"{input_path:?}: Expected parser to generate at least one syntax error for a program containing syntax errors." "{input_path:?}: Expected parser to generate at least one syntax error for a program containing syntax errors."
); );
@ -224,7 +220,7 @@ f'{foo!r'
println!("AST:\n----\n{:#?}", parsed.syntax()); println!("AST:\n----\n{:#?}", parsed.syntax());
println!("Tokens:\n-------\n{:#?}", parsed.tokens()); println!("Tokens:\n-------\n{:#?}", parsed.tokens());
if !parsed.is_valid() { if parsed.has_invalid_syntax() {
println!("Errors:\n-------"); println!("Errors:\n-------");
let line_index = LineIndex::from_source_text(source); let line_index = LineIndex::from_source_text(source);

View file

@ -135,7 +135,7 @@ fn do_fuzz(case: &[u8]) -> Corpus {
}; };
let parsed = parse_unchecked(code, ParseOptions::from(Mode::Module)); let parsed = parse_unchecked(code, ParseOptions::from(Mode::Module));
if parsed.is_valid() { if parsed.has_valid_syntax() {
return Corpus::Reject; return Corpus::Reject;
} }