Skip to content

Commit 497d425

Browse files
committed
Add DiagnosticReporter trait, deduplicate Unicode checks
Summary -- This PR is stacked on #19608 and of more dubious value. It bothered me to see that `Candidate::into_diagnostic` and `Candidate::report_diagnostic` were essentially the same, differing only in taking a `LintContext` instead of a `Checker`. This was also the only reason we needed to collect a `Vec` of `Candidate`s in `ambiguous_unicode_character` instead of reporting them directly. This PR fixes those two concerns, but at the cost of introducing a trait implemented by `LintContext` and `Checker`. I'm leaning towards it not being worth it, unless we think the trait will be useful elsewhere. If we do want to keep the trait, we could obviously move `report_diagnostic` into it and move the actual implementations into the trait impls. I held off doing that for now to avoid a big import diff, especially if we didn't want this change at all. Test Plan -- Existing tests
1 parent 01361d6 commit 497d425

File tree

2 files changed

+67
-65
lines changed

2 files changed

+67
-65
lines changed

crates/ruff_linter/src/checkers/ast/mod.rs

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3326,3 +3326,36 @@ impl Drop for DiagnosticGuard<'_, '_> {
33263326
}
33273327
}
33283328
}
3329+
3330+
pub(crate) trait DiagnosticReporter<'a> {
3331+
/// Return a [`DiagnosticGuard`] for reporting a diagnostic if the corresponding rule is
3332+
/// enabled.
3333+
///
3334+
/// The guard derefs to a [`Diagnostic`], so it can be used to further modify the diagnostic
3335+
/// before it is added to the collection in the context on `Drop`.
3336+
fn report_diagnostic_if_enabled<'chk, T: Violation>(
3337+
&'chk self,
3338+
kind: T,
3339+
range: TextRange,
3340+
) -> Option<DiagnosticGuard<'chk, 'a>>;
3341+
}
3342+
3343+
impl<'a> DiagnosticReporter<'a> for Checker<'a> {
3344+
fn report_diagnostic_if_enabled<'chk, T: Violation>(
3345+
&'chk self,
3346+
kind: T,
3347+
range: TextRange,
3348+
) -> Option<DiagnosticGuard<'chk, 'a>> {
3349+
self.report_diagnostic_if_enabled(kind, range)
3350+
}
3351+
}
3352+
3353+
impl<'a> DiagnosticReporter<'a> for LintContext<'a> {
3354+
fn report_diagnostic_if_enabled<'chk, T: Violation>(
3355+
&'chk self,
3356+
kind: T,
3357+
range: TextRange,
3358+
) -> Option<DiagnosticGuard<'chk, 'a>> {
3359+
self.report_diagnostic_if_enabled(kind, range)
3360+
}
3361+
}

crates/ruff_linter/src/rules/ruff/rules/ambiguous_unicode_character.rs

Lines changed: 34 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ use ruff_text_size::{Ranged, TextLen, TextRange, TextSize};
88

99
use crate::Locator;
1010
use crate::Violation;
11+
use crate::checkers::ast::DiagnosticReporter;
1112
use crate::checkers::ast::{Checker, LintContext};
1213
use crate::preview::is_unicode_to_unicode_confusables_enabled;
1314
use crate::rules::ruff::rules::Context;
@@ -180,9 +181,7 @@ pub(crate) fn ambiguous_unicode_character_comment(
180181
range: TextRange,
181182
) {
182183
let text = locator.slice(range);
183-
for candidate in ambiguous_unicode_character(text, range, context.settings()) {
184-
candidate.into_diagnostic(Context::Comment, context);
185-
}
184+
ambiguous_unicode_character(text, range, context.settings(), Context::Comment, context);
186185
}
187186

188187
/// RUF001, RUF002
@@ -203,38 +202,42 @@ pub(crate) fn ambiguous_unicode_character_string(checker: &Checker, string_like:
203202
match part {
204203
ast::StringLikePart::String(string_literal) => {
205204
let text = checker.locator().slice(string_literal);
206-
for candidate in
207-
ambiguous_unicode_character(text, string_literal.range(), checker.settings())
208-
{
209-
candidate.report_diagnostic(checker, context);
210-
}
205+
ambiguous_unicode_character(
206+
text,
207+
string_literal.range(),
208+
checker.settings(),
209+
context,
210+
checker,
211+
);
211212
}
212213
ast::StringLikePart::Bytes(_) => {}
213214
ast::StringLikePart::FString(FString { elements, .. })
214215
| ast::StringLikePart::TString(TString { elements, .. }) => {
215216
for literal in elements.literals() {
216217
let text = checker.locator().slice(literal);
217-
for candidate in
218-
ambiguous_unicode_character(text, literal.range(), checker.settings())
219-
{
220-
candidate.report_diagnostic(checker, context);
221-
}
218+
ambiguous_unicode_character(
219+
text,
220+
literal.range(),
221+
checker.settings(),
222+
context,
223+
checker,
224+
);
222225
}
223226
}
224227
}
225228
}
226229
}
227230

228-
fn ambiguous_unicode_character(
231+
fn ambiguous_unicode_character<'a>(
229232
text: &str,
230233
range: TextRange,
231234
settings: &LinterSettings,
232-
) -> Vec<Candidate> {
233-
let mut candidates = Vec::new();
234-
235+
context: Context,
236+
reporter: &impl DiagnosticReporter<'a>,
237+
) {
235238
// Most of the time, we don't need to check for ambiguous unicode characters at all.
236239
if text.is_ascii() {
237-
return candidates;
240+
return;
238241
}
239242

240243
// Iterate over the "words" in the text.
@@ -246,7 +249,7 @@ fn ambiguous_unicode_character(
246249
if !word_candidates.is_empty() {
247250
if word_flags.is_candidate_word() {
248251
for candidate in word_candidates.drain(..) {
249-
candidates.push(candidate);
252+
candidate.into_diagnostic(context, reporter, settings);
250253
}
251254
}
252255
word_candidates.clear();
@@ -264,7 +267,7 @@ fn ambiguous_unicode_character(
264267
current_char,
265268
representant,
266269
);
267-
candidates.push(candidate);
270+
candidate.into_diagnostic(context, reporter, settings);
268271
}
269272
}
270273
} else if current_char.is_ascii() {
@@ -289,13 +292,11 @@ fn ambiguous_unicode_character(
289292
if !word_candidates.is_empty() {
290293
if word_flags.is_candidate_word() {
291294
for candidate in word_candidates.drain(..) {
292-
candidates.push(candidate);
295+
candidate.into_diagnostic(context, reporter, settings);
293296
}
294297
}
295298
word_candidates.clear();
296299
}
297-
298-
candidates
299300
}
300301

301302
bitflags! {
@@ -341,62 +342,30 @@ impl Candidate {
341342
}
342343
}
343344

344-
fn into_diagnostic(self, context: Context, lint_context: &LintContext) {
345-
if !lint_context
346-
.settings()
347-
.allowed_confusables
348-
.contains(&self.confusable)
349-
{
350-
let char_range = TextRange::at(self.offset, self.confusable.text_len());
351-
match context {
352-
Context::String => lint_context.report_diagnostic_if_enabled(
353-
AmbiguousUnicodeCharacterString {
354-
confusable: self.confusable,
355-
representant: self.representant,
356-
},
357-
char_range,
358-
),
359-
Context::Docstring => lint_context.report_diagnostic_if_enabled(
360-
AmbiguousUnicodeCharacterDocstring {
361-
confusable: self.confusable,
362-
representant: self.representant,
363-
},
364-
char_range,
365-
),
366-
Context::Comment => lint_context.report_diagnostic_if_enabled(
367-
AmbiguousUnicodeCharacterComment {
368-
confusable: self.confusable,
369-
representant: self.representant,
370-
},
371-
char_range,
372-
),
373-
};
374-
}
375-
}
376-
377-
fn report_diagnostic(self, checker: &Checker, context: Context) {
378-
if !checker
379-
.settings()
380-
.allowed_confusables
381-
.contains(&self.confusable)
382-
{
345+
fn into_diagnostic<'a>(
346+
self,
347+
context: Context,
348+
reporter: &impl DiagnosticReporter<'a>,
349+
settings: &LinterSettings,
350+
) {
351+
if !settings.allowed_confusables.contains(&self.confusable) {
383352
let char_range = TextRange::at(self.offset, self.confusable.text_len());
384353
match context {
385-
Context::String => checker.report_diagnostic_if_enabled(
354+
Context::String => reporter.report_diagnostic_if_enabled(
386355
AmbiguousUnicodeCharacterString {
387356
confusable: self.confusable,
388357
representant: self.representant,
389358
},
390359
char_range,
391360
),
392-
Context::Docstring => checker.report_diagnostic_if_enabled(
361+
Context::Docstring => reporter.report_diagnostic_if_enabled(
393362
AmbiguousUnicodeCharacterDocstring {
394363
confusable: self.confusable,
395364
representant: self.representant,
396365
},
397366
char_range,
398367
),
399-
Context::Comment => checker.report_diagnostic_if_enabled(
368+
Context::Comment => reporter.report_diagnostic_if_enabled(
400369
AmbiguousUnicodeCharacterComment {
401370
confusable: self.confusable,
402371
representant: self.representant,

0 commit comments

Comments
 (0)