-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Infrastructure for lints during attribute parsing, specifically duplicate usages of attributes #138164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
This comment has been minimized.
This comment has been minimized.
(am aware of merge commits, was just an easy way to get those changes in before they're on master so we can run CI on this PR. will obviously remove once those changes are on master and this PR can be merged) |
This comment has been minimized.
This comment has been minimized.
b5f1130
to
9559a37
Compare
This comment has been minimized.
This comment has been minimized.
9559a37
to
d529827
Compare
This comment has been minimized.
This comment has been minimized.
d529827
to
bfeed70
Compare
This comment has been minimized.
This comment has been minimized.
bfeed70
to
fc63551
Compare
☔ The latest upstream changes (presumably #138388) made this pull request unmergeable. Please resolve the merge conflicts. |
2be9ade
to
57ee80c
Compare
@rustbot review |
lint on duplicates during attribute parsing To do this we stuff them in the diagnostic context to be emitted after hir is constructed
92205a2
to
e2afe04
Compare
@bors r=oli_obk |
@bors r=oli-obk |
💡 This pull request was already approved, no need to approve it again.
|
…ra, r=oli-obk Infrastructure for lints during attribute parsing, specifically duplicate usages of attributes r? `@oli-obk` This PR adds a new field to OwnerInfo to buffer lints which are generated during attribute parsing and ast lowering in general. They can't be emitted at this stage because at that point there's no HIR yet, and early lints are already emitted. This also adds the generic `S: Stage` to attribute parsers. Currently we don't emit any lints during early attribute parsing, but if we ever want to that logic will be different. That's because there we don't have hir ids yet, while at the same time still having access to node ids and early lints. Even though that logic isn't completely there in this PR (no worries, we don't use it), that's why the parameter is there. With this PR, we also add 2 associated consts to `SingleAttributeParser`. Those determine what logic should be applied when finding a duplicate attribute. This PR was getting pretty large, so the first code using this logic is in rust-lang#138165. This code is all new things that weren't possible before so it also doesn't break any behaviour. However, some of it will be dead code right now. I recommend reviewing both before merging, though in some sense that doubles the size of the review again, and the other PR might be more controversial. Let me know how you want to do this `@oli-obk`
Rollup of 10 pull requests Successful merges: - #134536 (Lint on fn pointers comparisons in external macros) - #138164 (Infrastructure for lints during attribute parsing, specifically duplicate usages of attributes) - #141069 (Suggest mut when possbile for temporary value dropped while borrowed) - #141934 (resolve: Tweak `private_macro_use` lint to be compatible with upcoming macro prelude changes) - #142034 (Detect method not being present that is present in other tuple types) - #142402 (chore(doctest): Remove redundant blank lines) - #142406 (Note when enum variants shadow an associated function) - #142407 (Remove bootstrap adhoc group) - #142408 (Add myself (WaffleLapkin) to review rotation) - #142418 (Remove lower_arg_ty as all callers were passing `None`) r? `@ghost` `@rustbot` modify labels: rollup
There are several open PRs touching adjacent areas that will conflict with this, and there is somewhat-timing-sensitive work that is strictly blocked on this. Bumping its priority accordingly. @bors p=1 rollup=iffy |
( Ideally it just lands in the rollup currently trying to land though. ) |
@workingjubilee I'm happy this gets merged but some of those I deliberately didn't r+ yet to wait for this one but this makes sure it work :) |
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 6c8138d (parent) -> 573a015 (this PR) Test differencesShow 147 test diffsStage 1
Additionally, 138 doctest diffs were found. These are ignored, as they are noisy. Job group index Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 573a01569000d395498a5f98f916d6e5305ac81a --output-dir test-dashboard And then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Finished benchmarking commit (573a015): comparison URL. Overall result: ❌ regressions - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.7%, secondary 3.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -1.0%, secondary 1.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 754.987s -> 754.801s (-0.02%) |
r? @oli-obk
This PR adds a new field to OwnerInfo to buffer lints which are generated during attribute parsing and ast lowering in general. They can't be emitted at this stage because at that point there's no HIR yet, and early lints are already emitted.
This also adds the generic
S: Stage
to attribute parsers. Currently we don't emit any lints during early attribute parsing, but if we ever want to that logic will be different. That's because there we don't have hir ids yet, while at the same time still having access to node ids and early lints. Even though that logic isn't completely there in this PR (no worries, we don't use it), that's why the parameter is there.With this PR, we also add 2 associated consts to
SingleAttributeParser
. Those determine what logic should be applied when finding a duplicate attribute.This PR was getting pretty large, so the first code using this logic is in #138165. This code is all new things that weren't possible before so it also doesn't break any behaviour. However, some of it will be dead code right now. I recommend reviewing both before merging, though in some sense that doubles the size of the review again, and the other PR might be more controversial. Let me know how you want to do this @oli-obk