Allow [absolute_paths] to be configured based on occurrence#16679
Allow [absolute_paths] to be configured based on occurrence#16679paulmialane wants to merge 1 commit intorust-lang:masterfrom
absolute_paths] to be configured based on occurrence#16679Conversation
Add `absolute-paths-max-occurrences` config option to `absolute_paths` Allow users to only trigger the lint when the same absolute path is used more than N times in a single file, rather than on every occurrence. The default value of 0 preserves the existing behavior of linting every occurrence unconditionally. Occurrences suppressed by `#[allow]` or `#[expect]` are excluded from the count entirely, so they do not contribute toward the threshold. Related issue : rust-lang#16574
rustbot
added
the
S-waiting-on-review
|
r? @Jarcho rustbot has assigned @Jarcho. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
| && !self.absolute_paths_allowed_crates.contains(&crate_name) | ||
| && !self.allowed_crates.contains(&crate_name) | ||
| && !is_from_proc_macro(cx, path) | ||
| && !fulfill_or_allowed(cx, ABSOLUTE_PATHS, [hir_id]) |
There was a problem hiding this comment.
This should only run if max_occurrences > 0. This is slower to check than the allow and expect filtering that occurs in span_lint.
| } else if let Res::Def(_, def_id) = path.res { | ||
| // Occurence based behaviour : accumulate spans and emit lint in check_crate_post | ||
| // if the occurence count is exceeded | ||
| let file = cx.tcx.sess.source_map().lookup_source_file(path.span.lo()).name.clone(); |
There was a problem hiding this comment.
You should not be using the file, but the containing module. You can use hir_parent_owner_iter until you hit a module or the crate.
| for ((_def_id, _file), spans_vec) in self.occurrences.drain() { | ||
| if spans_vec.len() as u64 > self.max_occurrences { | ||
| for span in spans_vec { | ||
| span_lint( | ||
| cx, | ||
| ABSOLUTE_PATHS, | ||
| span, | ||
| "this absolute path is used too many times in this file, consider bringing it into scope with the `use` keyword", | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This doesn't have a stable lint emission order. You need to collect and then sort the spans before you can emit them.
| pub max_segments: u64, | ||
| pub allowed_crates: FxHashSet<Symbol>, | ||
| pub max_occurrences: u64, | ||
| occurrences: FxHashMap<(DefId, FileName), Vec<Span>>, // To track count of occurences |
There was a problem hiding this comment.
This would be better collected as Vec<(Key, Value)> . At the end you can the sort by key, split into chunks by key, and only emit based on each chunk's length.
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
|
You'll need to remove the link to the issue from the commit message. They cause GitHub to spam that issue a fair bit. |
3 participants
fixes #16574
Add
absolute-paths-max-occurrencesconfig option to [absolute_paths]Allow users to only trigger the lint when the same absolute path is used
more than N times in a single file, rather than on every occurrence. The
default value of 0 preserves the existing behavior of linting every
occurrence unconditionally.
Occurrences suppressed by
#[allow]or#[expect]are excluded fromthe count entirely, so they do not contribute toward the threshold.
I have two small problems :
tests/ui-cargowith a dedicated crate. My question is : should I move the other existing tests there ?changelog: [
absolute_paths]: Addabsolute-paths-max-occurrencesconfig option