Skip to content

Commit

Permalink
Simplify LinterResult, avoid cloning ParseError (#11903)
Browse files Browse the repository at this point in the history
## Summary

Follow-up to #11902

This PR simplifies the `LinterResult` struct by avoiding the generic and
not store the `ParseError`.

This is possible because the callers already have access to the
`ParseError` via the `Parsed` output. This also means that we can
simplify the return type of `check_path` and avoid the generic `T` on
`LinterResult`.

## Test Plan

`cargo insta test`
  • Loading branch information
dhruvmanila committed Jun 27, 2024
1 parent c811844 commit efc4114
Show file tree
Hide file tree
Showing 10 changed files with 96 additions and 99 deletions.
8 changes: 4 additions & 4 deletions crates/ruff/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -264,8 +264,8 @@ pub(crate) fn lint_path(
// Lint the file.
let (
LinterResult {
data: messages,
error: parse_error,
messages,
has_syntax_error: has_error,
},
transformed,
fixed,
Expand Down Expand Up @@ -334,7 +334,7 @@ pub(crate) fn lint_path(

if let Some((cache, relative_path, key)) = caching {
// We don't cache parsing errors.
if parse_error.is_none() {
if !has_error {
// `FixMode::Apply` and `FixMode::Diff` rely on side-effects (writing to disk,
// and writing the diff to stdout, respectively). If a file has diagnostics, we
// need to avoid reading from and writing to the cache in these modes.
Expand Down Expand Up @@ -400,7 +400,7 @@ pub(crate) fn lint_stdin(
};

// Lint the inputs.
let (LinterResult { data: messages, .. }, transformed, fixed) =
let (LinterResult { messages, .. }, transformed, fixed) =
if matches!(fix_mode, flags::FixMode::Apply | flags::FixMode::Diff) {
if let Ok(FixerResult {
result,
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_benchmark/benches/linter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ fn benchmark_linter(mut group: BenchmarkGroup, settings: &LinterSettings) {
);

// Assert that file contains no parse errors
assert_eq!(result.error, None);
assert!(!result.has_syntax_error);
},
criterion::BatchSize::SmallInput,
);
Expand Down
92 changes: 48 additions & 44 deletions crates/ruff_linter/src/linter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,29 +35,19 @@ use crate::settings::{flags, LinterSettings};
use crate::source_kind::SourceKind;
use crate::{directives, fs};

/// A [`Result`]-like type that returns both data and an error. Used to return
/// diagnostics even in the face of parse errors, since many diagnostics can be
/// generated without a full AST.
pub struct LinterResult<T> {
pub data: T,
pub error: Option<ParseError>,
}

impl<T> LinterResult<T> {
const fn new(data: T, error: Option<ParseError>) -> Self {
Self { data, error }
}

fn map<U, F: FnOnce(T) -> U>(self, f: F) -> LinterResult<U> {
LinterResult::new(f(self.data), self.error)
}
pub struct LinterResult {
/// A collection of diagnostic messages generated by the linter.
pub messages: Vec<Message>,
/// 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.
pub has_syntax_error: bool,
}

pub type FixTable = FxHashMap<Rule, usize>;

pub struct FixerResult<'a> {
/// The result returned by the linter, after applying any fixes.
pub result: LinterResult<Vec<Message>>,
pub result: LinterResult,
/// The resulting source code, after applying any fixes.
pub transformed: Cow<'a, SourceKind>,
/// The number of fixes applied for each [`Rule`].
Expand All @@ -79,7 +69,7 @@ pub fn check_path(
source_kind: &SourceKind,
source_type: PySourceType,
parsed: &Parsed<ModModule>,
) -> LinterResult<Vec<Diagnostic>> {
) -> Vec<Diagnostic> {
// Aggregate all diagnostics.
let mut diagnostics = vec![];

Expand Down Expand Up @@ -317,7 +307,7 @@ pub fn check_path(
}
}

LinterResult::new(diagnostics, parsed.errors().iter().next().cloned())
diagnostics
}

const MAX_ITERATIONS: usize = 100;
Expand Down Expand Up @@ -351,9 +341,7 @@ pub fn add_noqa_to_path(
);

// Generate diagnostics, ignoring any existing `noqa` directives.
let LinterResult {
data: diagnostics, ..
} = check_path(
let diagnostics = check_path(
path,
package,
&locator,
Expand Down Expand Up @@ -390,7 +378,7 @@ pub fn lint_only(
source_kind: &SourceKind,
source_type: PySourceType,
source: ParseSource,
) -> LinterResult<Vec<Message>> {
) -> LinterResult {
let parsed = source.into_parsed(source_kind, source_type);

// Map row and column locations to byte slices (lazily).
Expand All @@ -411,7 +399,7 @@ pub fn lint_only(
);

// Generate diagnostics.
let result = check_path(
let diagnostics = check_path(
path,
package,
&locator,
Expand All @@ -425,9 +413,16 @@ pub fn lint_only(
&parsed,
);

result.map(|diagnostics| {
diagnostics_to_messages(diagnostics, parsed.errors(), path, &locator, &directives)
})
LinterResult {
messages: diagnostics_to_messages(
diagnostics,
parsed.errors(),
path,
&locator,
&directives,
),
has_syntax_error: !parsed.is_valid(),
}
}

/// Convert from diagnostics to messages.
Expand Down Expand Up @@ -479,8 +474,8 @@ pub fn lint_fix<'a>(
// As an escape hatch, bail after 100 iterations.
let mut iterations = 0;

// Track whether the _initial_ source code was parseable.
let mut parseable = false;
// Track whether the _initial_ source code is valid syntax.
let mut is_valid_syntax = false;

// Continuously fix until the source code stabilizes.
loop {
Expand All @@ -506,7 +501,7 @@ pub fn lint_fix<'a>(
);

// Generate diagnostics.
let result = check_path(
let diagnostics = check_path(
path,
package,
&locator,
Expand All @@ -521,19 +516,21 @@ pub fn lint_fix<'a>(
);

if iterations == 0 {
parseable = result.error.is_none();
is_valid_syntax = parsed.is_valid();
} else {
// If the source code was parseable on the first pass, but is no
// longer parseable on a subsequent pass, then we've introduced a
// syntax error. Return the original code.
if parseable && result.error.is_some() {
report_fix_syntax_error(
path,
transformed.source_code(),
&result.error.unwrap(),
fixed.keys().copied(),
);
return Err(anyhow!("Fix introduced a syntax error"));
if is_valid_syntax {
if let Some(error) = parsed.errors().first() {
report_fix_syntax_error(
path,
transformed.source_code(),
error,
fixed.keys().copied(),
);
return Err(anyhow!("Fix introduced a syntax error"));
}
}
}

Expand All @@ -542,7 +539,7 @@ pub fn lint_fix<'a>(
code: fixed_contents,
fixes: applied,
source_map,
}) = fix_file(&result.data, &locator, unsafe_fixes)
}) = fix_file(&diagnostics, &locator, unsafe_fixes)
{
if iterations < MAX_ITERATIONS {
// Count the number of fixed errors.
Expand All @@ -559,13 +556,20 @@ pub fn lint_fix<'a>(
continue;
}

report_failed_to_converge_error(path, transformed.source_code(), &result.data);
report_failed_to_converge_error(path, transformed.source_code(), &diagnostics);
}

return Ok(FixerResult {
result: result.map(|diagnostics| {
diagnostics_to_messages(diagnostics, parsed.errors(), path, &locator, &directives)
}),
result: LinterResult {
messages: diagnostics_to_messages(
diagnostics,
parsed.errors(),
path,
&locator,
&directives,
),
has_syntax_error: !is_valid_syntax,
},
transformed,
fixed,
});
Expand Down
7 changes: 2 additions & 5 deletions crates/ruff_linter/src/rules/pyflakes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ mod tests {
use ruff_source_file::Locator;
use ruff_text_size::Ranged;

use crate::linter::{check_path, LinterResult};
use crate::linter::check_path;
use crate::registry::{AsRule, Linter, Rule};
use crate::rules::pyflakes;
use crate::settings::types::PreviewMode;
Expand Down Expand Up @@ -650,10 +650,7 @@ mod tests {
&locator,
&indexer,
);
let LinterResult {
data: mut diagnostics,
..
} = check_path(
let mut diagnostics = check_path(
Path::new("<filename>"),
None,
&locator,
Expand Down
14 changes: 4 additions & 10 deletions crates/ruff_linter/src/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use ruff_text_size::Ranged;

use crate::directives;
use crate::fix::{fix_file, FixResult};
use crate::linter::{check_path, LinterResult};
use crate::linter::check_path;
use crate::message::{Emitter, EmitterContext, Message, TextEmitter};
use crate::packaging::detect_package_root;
use crate::registry::AsRule;
Expand Down Expand Up @@ -119,10 +119,7 @@ pub(crate) fn test_contents<'a>(
&locator,
&indexer,
);
let LinterResult {
data: diagnostics,
error,
} = check_path(
let diagnostics = check_path(
path,
path.parent()
.and_then(|parent| detect_package_root(parent, &settings.namespace_packages)),
Expand All @@ -137,7 +134,7 @@ pub(crate) fn test_contents<'a>(
&parsed,
);

let source_has_errors = error.is_some();
let source_has_errors = !parsed.is_valid();

// Detect fixes that don't converge after multiple iterations.
let mut iterations = 0;
Expand Down Expand Up @@ -186,10 +183,7 @@ pub(crate) fn test_contents<'a>(
&indexer,
);

let LinterResult {
data: fixed_diagnostics,
..
} = check_path(
let fixed_diagnostics = check_path(
path,
None,
&locator,
Expand Down
11 changes: 6 additions & 5 deletions crates/ruff_server/src/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,10 @@ pub(crate) fn fix_all(
// which is inconsistent with how `ruff check --fix` works.
let FixerResult {
transformed,
result: LinterResult { error, .. },
result: LinterResult {
has_syntax_error: has_error,
..
},
..
} = ruff_linter::linter::lint_fix(
&query.virtual_file_path(),
Expand All @@ -80,11 +83,9 @@ pub(crate) fn fix_all(
source_type,
)?;

if let Some(error) = error {
if has_error {
// abort early if a parsing error occurred
return Err(anyhow::anyhow!(
"A parsing error occurred during `fix_all`: {error}"
));
return Err(anyhow::anyhow!("A parsing error occurred during `fix_all`"));
}

// fast path: if `transformed` is still borrowed, no changes were made and we can return early
Expand Down
Loading

0 comments on commit efc4114

Please sign in to comment.