Skip to content

Commit 72b6c26

Browse files
dhruvmanilaMichaReiser
dhruvmanila
authored and
MichaReiser
committed
Simplify LinterResult, avoid cloning ParseError (#11903)
## 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`
1 parent 73851e7 commit 72b6c26

10 files changed

Lines changed: 96 additions & 99 deletions

File tree

‎crates/ruff/src/diagnostics.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -264,8 +264,8 @@ pub(crate) fn lint_path(
264264
// Lint the file.
265265
let (
266266
LinterResult {
267-
data: messages,
268-
error: parse_error,
267+
messages,
268+
has_syntax_error: has_error,
269269
},
270270
transformed,
271271
fixed,
@@ -334,7 +334,7 @@ pub(crate) fn lint_path(
334334

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

402402
// Lint the inputs.
403-
let (LinterResult { data: messages, .. }, transformed, fixed) =
403+
let (LinterResult { messages, .. }, transformed, fixed) =
404404
if matches!(fix_mode, flags::FixMode::Apply | flags::FixMode::Diff) {
405405
if let Ok(FixerResult {
406406
result,

‎crates/ruff_benchmark/benches/linter.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ fn benchmark_linter(mut group: BenchmarkGroup, settings: &LinterSettings) {
7373
);
7474

7575
// Assert that file contains no parse errors
76-
assert_eq!(result.error, None);
76+
assert!(!result.has_syntax_error);
7777
},
7878
criterion::BatchSize::SmallInput,
7979
);

‎crates/ruff_linter/src/linter.rs‎

Lines changed: 48 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -35,29 +35,19 @@ use crate::settings::{flags, LinterSettings};
3535
use crate::source_kind::SourceKind;
3636
use crate::{directives, fs};
3737

38-
/// A [`Result`]-like type that returns both data and an error. Used to return
39-
/// diagnostics even in the face of parse errors, since many diagnostics can be
40-
/// generated without a full AST.
41-
pub struct LinterResult<T> {
42-
pub data: T,
43-
pub error: Option<ParseError>,
44-
}
45-
46-
impl<T> LinterResult<T> {
47-
const fn new(data: T, error: Option<ParseError>) -> Self {
48-
Self { data, error }
49-
}
50-
51-
fn map<U, F: FnOnce(T) -> U>(self, f: F) -> LinterResult<U> {
52-
LinterResult::new(f(self.data), self.error)
53-
}
38+
pub struct LinterResult {
39+
/// A collection of diagnostic messages generated by the linter.
40+
pub messages: Vec<Message>,
41+
/// A flag indicating the presence of syntax errors in the source file.
42+
/// If `true`, at least one syntax error was detected in the source file.
43+
pub has_syntax_error: bool,
5444
}
5545

5646
pub type FixTable = FxHashMap<Rule, usize>;
5747

5848
pub struct FixerResult<'a> {
5949
/// The result returned by the linter, after applying any fixes.
60-
pub result: LinterResult<Vec<Message>>,
50+
pub result: LinterResult,
6151
/// The resulting source code, after applying any fixes.
6252
pub transformed: Cow<'a, SourceKind>,
6353
/// The number of fixes applied for each [`Rule`].
@@ -79,7 +69,7 @@ pub fn check_path(
7969
source_kind: &SourceKind,
8070
source_type: PySourceType,
8171
parsed: &Parsed<ModModule>,
82-
) -> LinterResult<Vec<Diagnostic>> {
72+
) -> Vec<Diagnostic> {
8373
// Aggregate all diagnostics.
8474
let mut diagnostics = vec![];
8575

@@ -317,7 +307,7 @@ pub fn check_path(
317307
}
318308
}
319309

320-
LinterResult::new(diagnostics, parsed.errors().iter().next().cloned())
310+
diagnostics
321311
}
322312

323313
const MAX_ITERATIONS: usize = 100;
@@ -351,9 +341,7 @@ pub fn add_noqa_to_path(
351341
);
352342

353343
// Generate diagnostics, ignoring any existing `noqa` directives.
354-
let LinterResult {
355-
data: diagnostics, ..
356-
} = check_path(
344+
let diagnostics = check_path(
357345
path,
358346
package,
359347
&locator,
@@ -390,7 +378,7 @@ pub fn lint_only(
390378
source_kind: &SourceKind,
391379
source_type: PySourceType,
392380
source: ParseSource,
393-
) -> LinterResult<Vec<Message>> {
381+
) -> LinterResult {
394382
let parsed = source.into_parsed(source_kind, source_type);
395383

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

413401
// Generate diagnostics.
414-
let result = check_path(
402+
let diagnostics = check_path(
415403
path,
416404
package,
417405
&locator,
@@ -425,9 +413,16 @@ pub fn lint_only(
425413
&parsed,
426414
);
427415

428-
result.map(|diagnostics| {
429-
diagnostics_to_messages(diagnostics, parsed.errors(), path, &locator, &directives)
430-
})
416+
LinterResult {
417+
messages: diagnostics_to_messages(
418+
diagnostics,
419+
parsed.errors(),
420+
path,
421+
&locator,
422+
&directives,
423+
),
424+
has_syntax_error: !parsed.is_valid(),
425+
}
431426
}
432427

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

482-
// Track whether the _initial_ source code was parseable.
483-
let mut parseable = false;
477+
// Track whether the _initial_ source code is valid syntax.
478+
let mut is_valid_syntax = false;
484479

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

508503
// Generate diagnostics.
509-
let result = check_path(
504+
let diagnostics = check_path(
510505
path,
511506
package,
512507
&locator,
@@ -521,19 +516,21 @@ pub fn lint_fix<'a>(
521516
);
522517

523518
if iterations == 0 {
524-
parseable = result.error.is_none();
519+
is_valid_syntax = parsed.is_valid();
525520
} else {
526521
// If the source code was parseable on the first pass, but is no
527522
// longer parseable on a subsequent pass, then we've introduced a
528523
// syntax error. Return the original code.
529-
if parseable && result.error.is_some() {
530-
report_fix_syntax_error(
531-
path,
532-
transformed.source_code(),
533-
&result.error.unwrap(),
534-
fixed.keys().copied(),
535-
);
536-
return Err(anyhow!("Fix introduced a syntax error"));
524+
if is_valid_syntax {
525+
if let Some(error) = parsed.errors().first() {
526+
report_fix_syntax_error(
527+
path,
528+
transformed.source_code(),
529+
error,
530+
fixed.keys().copied(),
531+
);
532+
return Err(anyhow!("Fix introduced a syntax error"));
533+
}
537534
}
538535
}
539536

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

562-
report_failed_to_converge_error(path, transformed.source_code(), &result.data);
559+
report_failed_to_converge_error(path, transformed.source_code(), &diagnostics);
563560
}
564561

565562
return Ok(FixerResult {
566-
result: result.map(|diagnostics| {
567-
diagnostics_to_messages(diagnostics, parsed.errors(), path, &locator, &directives)
568-
}),
563+
result: LinterResult {
564+
messages: diagnostics_to_messages(
565+
diagnostics,
566+
parsed.errors(),
567+
path,
568+
&locator,
569+
&directives,
570+
),
571+
has_syntax_error: !is_valid_syntax,
572+
},
569573
transformed,
570574
fixed,
571575
});

‎crates/ruff_linter/src/rules/pyflakes/mod.rs‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ mod tests {
2222
use ruff_source_file::Locator;
2323
use ruff_text_size::Ranged;
2424

25-
use crate::linter::{check_path, LinterResult};
25+
use crate::linter::check_path;
2626
use crate::registry::{AsRule, Linter, Rule};
2727
use crate::rules::pyflakes;
2828
use crate::settings::types::PreviewMode;
@@ -650,10 +650,7 @@ mod tests {
650650
&locator,
651651
&indexer,
652652
);
653-
let LinterResult {
654-
data: mut diagnostics,
655-
..
656-
} = check_path(
653+
let mut diagnostics = check_path(
657654
Path::new("<filename>"),
658655
None,
659656
&locator,

‎crates/ruff_linter/src/test.rs‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ use ruff_text_size::Ranged;
2323

2424
use crate::directives;
2525
use crate::fix::{fix_file, FixResult};
26-
use crate::linter::{check_path, LinterResult};
26+
use crate::linter::check_path;
2727
use crate::message::{Emitter, EmitterContext, Message, TextEmitter};
2828
use crate::packaging::detect_package_root;
2929
use crate::registry::AsRule;
@@ -119,10 +119,7 @@ pub(crate) fn test_contents<'a>(
119119
&locator,
120120
&indexer,
121121
);
122-
let LinterResult {
123-
data: diagnostics,
124-
error,
125-
} = check_path(
122+
let diagnostics = check_path(
126123
path,
127124
path.parent()
128125
.and_then(|parent| detect_package_root(parent, &settings.namespace_packages)),
@@ -137,7 +134,7 @@ pub(crate) fn test_contents<'a>(
137134
&parsed,
138135
);
139136

140-
let source_has_errors = error.is_some();
137+
let source_has_errors = !parsed.is_valid();
141138

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

189-
let LinterResult {
190-
data: fixed_diagnostics,
191-
..
192-
} = check_path(
186+
let fixed_diagnostics = check_path(
193187
path,
194188
None,
195189
&locator,

‎crates/ruff_server/src/fix.rs‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,10 @@ pub(crate) fn fix_all(
6868
// which is inconsistent with how `ruff check --fix` works.
6969
let FixerResult {
7070
transformed,
71-
result: LinterResult { error, .. },
71+
result: LinterResult {
72+
has_syntax_error: has_error,
73+
..
74+
},
7275
..
7376
} = ruff_linter::linter::lint_fix(
7477
&query.virtual_file_path(),
@@ -80,11 +83,9 @@ pub(crate) fn fix_all(
8083
source_type,
8184
)?;
8285

83-
if let Some(error) = error {
86+
if has_error {
8487
// abort early if a parsing error occurred
85-
return Err(anyhow::anyhow!(
86-
"A parsing error occurred during `fix_all`: {error}"
87-
));
88+
return Err(anyhow::anyhow!("A parsing error occurred during `fix_all`"));
8889
}
8990

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

0 commit comments

Comments
 (0)