-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Implement another internal lints #61545
Conversation
Untested: |
The relevant code for this is in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I did not foresee the fallout that TyCtxtAt
would have. I'm going to review those in detail some time this week.
src/librustc/lint/internal.rs
Outdated
impl EarlyLintPass for LintPassImpl { | ||
fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) { | ||
if let ItemKind::Impl(_, _, _, _, Some(lint_pass), _, _) = &item.node { | ||
if !lint_pass.path.span.ctxt().outer_expn_info().is_some() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess any macro is better than none, checking for declare_lint_pass!
or impl_lint_pass!
specifically is overkill?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't thought of specifically checking for these macros. I'll change that (since it's not much work), but don't think that there will be more lint findings.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☔ The latest upstream changes (presumably #57428) made this pull request unmergeable. Please resolve the merge conflicts. |
Ok, so the current fallout seems overly excessive, I did not anticipate that. How hard do you think it would be to change the lint to only complain about functions that have both |
That would just require to add a visitor for the function body searching for calls to On the other hand, I just searched for "tcx.at" with the GitHub search and there would be just about 5 functions that would trigger this lint then. |
Ah yes, I expect that not much will trigger this at the moment, but changing those functions' APIs may again cause other functions to trigger the lint and so on. I guess it's not that important. Lets just remove the lint from the ideas list, it's too prone to false positives or weird situations |
I removed the commits for the Now only the TODOs are left to implement. |
I tried to enable
The alternative would be to leave it as is and activate internal lints crate wise. |
This is probably too hard to do. We'd need access to the dependencies, which we don't have yet while adding the lint to the list of lints to run (we need a
Imo keeping this on a crate-by-crate basis is ok. |
@flip1995 On a related note, it would be nice to rename |
We have lint tools, so we could even have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On moving the -Winternal
to bootstrap the two crates
libfmt_macros
libarena
don't get linted anymore. Is this okay or should they be added specifically to bootstrap?
src/librustc_macros/src/lib.rs
Outdated
@@ -1,5 +1,6 @@ | |||
#![feature(proc_macro_hygiene)] | |||
#![deny(rust_2018_idioms)] | |||
#![allow(default_hash_types)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we allow default_hash_types
here or use rustc_data_structures
in rustc_macros
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, is rustc_data_structures
using rustc_macros
for proc macros?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope rustc_data_structures
doesn't depend on rustc_macros
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, adding a further dependency to rustc_macros
seems fine to me then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, measure the build time.
Speedups from using FxHashMap
in the proc macros are unlikely to outweigh the build parallelism lost due to the introduced dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. HashSet
is only used once in rustc_macros
to verify, that no symbols are duplicated.
rust/src/librustc_macros/src/symbols.rs
Lines 102 to 108 in 0376941
let mut keys = HashSet::<String>::new(); | |
let mut check_dup = |str: &str| { | |
if !keys.insert(str.to_string()) { | |
panic!("Symbol `{}` is duplicated", str); | |
} | |
}; |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Spurious? 🤔 This PR doesn't add an |
Probably another spurious RLS test #62225 |
ping @oli-obk. Anything I can do here? I already have the next internal lint in the pipeline, but this needs to get merged first 😄 |
@bors r=oli-obk |
💡 This pull request was already approved, no need to approve it again.
|
📌 Commit d0625a3 has been approved by |
⌛ Testing commit d0625a3 with merge d8999f8dfb552c48656537a6c8bbe4da284d0a52... |
@bors retry yielding to r0llup. |
Implement another internal lints cc rust-lang#49509 This adds ~~two~~ one internal lint~~s~~: 1. LINT_PASS_IMPL_WITHOUT_MACRO: Make sure, that the `{declare,impl}_lint_pass` macro is used to implement lint passes. cc rust-lang#59669 2. ~~USAGE_OF_TYCTXT_AND_SPAN_ARGS: item 2 on the list in rust-lang#49509~~ ~~With 2. I wasn't sure, if this lint should be applied everywhere. That means a careful review of 0955835 would be great. Also 73fb9b4 allows this lint on some functions. Should I also apply this lint there?~~ TODO (not directly relevant for review): - [ ] rust-lang#59316 (comment) (not sure yet, if this works or how to query for `rustc_private`, since it's not in [`Features`](https://doc.rust-lang.org/nightly/nightly-rustc/syntax/feature_gate/struct.Features.html) 🤔 cc @eddyb) - [x] rust-lang#61735 (comment) - [x] Check explicitly for the `{declare,impl}_lint_pass!` macros r? @oli-obk
Implement another internal lints cc rust-lang#49509 This adds ~~two~~ one internal lint~~s~~: 1. LINT_PASS_IMPL_WITHOUT_MACRO: Make sure, that the `{declare,impl}_lint_pass` macro is used to implement lint passes. cc rust-lang#59669 2. ~~USAGE_OF_TYCTXT_AND_SPAN_ARGS: item 2 on the list in rust-lang#49509~~ ~~With 2. I wasn't sure, if this lint should be applied everywhere. That means a careful review of 0955835 would be great. Also 73fb9b4 allows this lint on some functions. Should I also apply this lint there?~~ TODO (not directly relevant for review): - [ ] rust-lang#59316 (comment) (not sure yet, if this works or how to query for `rustc_private`, since it's not in [`Features`](https://doc.rust-lang.org/nightly/nightly-rustc/syntax/feature_gate/struct.Features.html) 🤔 cc @eddyb) - [x] rust-lang#61735 (comment) - [x] Check explicitly for the `{declare,impl}_lint_pass!` macros r? @oli-obk
Implement another internal lints cc rust-lang#49509 This adds ~~two~~ one internal lint~~s~~: 1. LINT_PASS_IMPL_WITHOUT_MACRO: Make sure, that the `{declare,impl}_lint_pass` macro is used to implement lint passes. cc rust-lang#59669 2. ~~USAGE_OF_TYCTXT_AND_SPAN_ARGS: item 2 on the list in rust-lang#49509~~ ~~With 2. I wasn't sure, if this lint should be applied everywhere. That means a careful review of 0955835 would be great. Also 73fb9b4 allows this lint on some functions. Should I also apply this lint there?~~ TODO (not directly relevant for review): - [ ] rust-lang#59316 (comment) (not sure yet, if this works or how to query for `rustc_private`, since it's not in [`Features`](https://doc.rust-lang.org/nightly/nightly-rustc/syntax/feature_gate/struct.Features.html) 🤔 cc @eddyb) - [x] rust-lang#61735 (comment) - [x] Check explicitly for the `{declare,impl}_lint_pass!` macros r? @oli-obk
Rollup of 13 pull requests Successful merges: - #61545 (Implement another internal lints) - #62110 (Improve -Ztime-passes) - #62133 (Feature gate `rustc` attributes harder) - #62158 (Add MemoryExtra in InterpretCx constructor params) - #62168 (The (almost) culmination of HirIdification) - #62193 (Create async version of the dynamic-drop test) - #62369 (Remove `compile-pass` from compiletest) - #62380 (rustc_target: avoid negative register counts in the SysV x86_64 ABI.) - #62381 (Fix a typo in Write::write_vectored docs) - #62390 (Update README.md) - #62396 (remove Scalar::is_null_ptr) - #62406 (Lint on invalid values passed to x.py --warnings) - #62414 (Remove last use of mem::uninitialized in SGX) Failed merges: r? @ghost
☔ The latest upstream changes (presumably #62419) made this pull request unmergeable. Please resolve the merge conflicts. |
…lacrum rustbuild: Cleanup global lint settings Lint settings do not depend on `if let Some(target) = target` in any way, so they are moved out of that clause. Internal lints now respect `RUSTC_DENY_WARNINGS`. Crate name treatment is cleaned up a bit. cc rust-lang#61545 @flip1995 r? @Mark-Simulacrum
cc #49509
This adds
twoone internal lints:{declare,impl}_lint_pass
macro is used to implement lint passes. cc Reduce repetition in librustc(_lint) wrt. impl LintPass by using macros #59669USAGE_OF_TYCTXT_AND_SPAN_ARGS: item 2 on the list in Add compiler-internal lints #49509With 2. I wasn't sure, if this lint should be applied everywhere. That means a careful review of 0955835 would be great. Also 73fb9b4 allows this lint on some functions. Should I also apply this lint there?TODO (not directly relevant for review):
rustc_private
, since it's not inFeatures
🤔 cc @eddyb){declare,impl}_lint_pass!
macrosr? @oli-obk