Skip to content
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

Add compiler-internal lints #49509

Open
4 of 10 tasks
oli-obk opened this issue Mar 30, 2018 · 16 comments
Open
4 of 10 tasks

Add compiler-internal lints #49509

oli-obk opened this issue Mar 30, 2018 · 16 comments
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. S-tracking-impl-incomplete Status: The implementation is incomplete. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Mar 30, 2018

There are various "patterns" we don't want in the compiler out of one reason or another. I'm using this issue to collect them and create lints for detecting those patterns. The lints will only be available while building the compiler via a -Z flag. The flag will be enabled by the bootstrap script, so we don't have to worry about accidentally running them for users.

@oli-obk oli-obk added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 30, 2018
@oli-obk oli-obk self-assigned this Mar 30, 2018
@leonardo-m
Copy link

Why are HashSet and HashMap forbidden as dog food for the compiler? And why aren't they "forbidden" for user code? This seems a hint.

@Ixrec
Copy link
Contributor

Ixrec commented Mar 30, 2018

If I remember correctly, it's because the standard HashSet/Map make a significant effort to protect against DoS attacks, i.e. they try to make it impossible for an attacker to induce huge performance cliffs with special sequences of malicious inputs (for example: https://accidentallyquadratic.tumblr.com/post/153545455987/rust-hash-iteration-reinsertion), but that concern doesn't apply inside the compiler so it makes more sense to use a faster implementation that does not offer any such protection.

@XAMPPRocky XAMPPRocky added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Jun 29, 2018
@eddyb
Copy link
Member

eddyb commented Dec 4, 2018

@Ixrec Also, it's useful to have deterministic compilation, without sources of randomness.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 30, 2019

cc rust-lang/rust-clippy#3724

Implementing this lint in clippy will make rustc miss out on it. On the other hand, we could setup clippy to run on rustc similar to tidy

@flip1995 flip1995 mentioned this issue Mar 20, 2019
26 tasks
Centril added a commit to Centril/rust that referenced this issue Apr 3, 2019
…li-obk

Internal lints take 2

cc rust-lang#58701
cc rust-lang#49509

TODO: Add `#![warn(internal)]` to crates (and fix violations)

Crates depending on `rustc_data_structures`

- [x] librustc_resolve
- [x] librustc_driver
- [x] librustc_passes
- [x] librustc_metadata
- [x] librustc_interface
- [x] librustc_save_analysis
- [x] librustc_lint
- [x] librustc
- [x] librustc_incremental
- [x] librustc_codegen_utils
- [x] libarena
- [x] librustc_target
- [x] librustc_allocator
- [x] librustc_privacy
- [x] librustc_traits
- [x] librustc_borrowck
- [x] libsyntax
- [x] librustc_codegen_ssa
- [x] libsyntax_ext
- [x] librustc_errors
- [x] librustc_mir
- [x] libsyntax_pos
- [x] librustc_typeck

Crates with `feature(rustc_private)`
Excluding crates, which are already in the list above. Also excluding tools and tests.

- [ ] ~~libstd~~
- [x] libfmt_macros
- [x] librustdoc

r? @oli-obk
Centril added a commit to Centril/rust that referenced this issue Apr 3, 2019
…li-obk

Internal lints take 2

cc rust-lang#58701
cc rust-lang#49509

TODO: Add `#![warn(internal)]` to crates (and fix violations)

Crates depending on `rustc_data_structures`

- [x] librustc_resolve
- [x] librustc_driver
- [x] librustc_passes
- [x] librustc_metadata
- [x] librustc_interface
- [x] librustc_save_analysis
- [x] librustc_lint
- [x] librustc
- [x] librustc_incremental
- [x] librustc_codegen_utils
- [x] libarena
- [x] librustc_target
- [x] librustc_allocator
- [x] librustc_privacy
- [x] librustc_traits
- [x] librustc_borrowck
- [x] libsyntax
- [x] librustc_codegen_ssa
- [x] libsyntax_ext
- [x] librustc_errors
- [x] librustc_mir
- [x] libsyntax_pos
- [x] librustc_typeck

Crates with `feature(rustc_private)`
Excluding crates, which are already in the list above. Also excluding tools and tests.

- [ ] ~~libstd~~
- [x] libfmt_macros
- [x] librustdoc

r? @oli-obk
@flip1995
Copy link
Member

flip1995 commented Apr 4, 2019

The first and the last two items can be checked.

About the Clone on Copy lint: Would it make sense to uplift this lint completely to rustc? What's the state of the "no new lints in rustc" rule?

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 4, 2019

That rule still applies, can we move it to rustc as an internal lint but have clippy "reexport" it as its own lint?

@flip1995
Copy link
Member

flip1995 commented Apr 4, 2019

This could be possible by making the LintPass public in rustc and re-registering the LintPass in Clippy. I'm curious how this will work with tool_lints 🤔

@oli-obk

This comment has been minimized.

@eddyb
Copy link
Member

eddyb commented May 29, 2019

Hmm, instead of "don't do X outside of the Y module" - can we simply allow it, not on the whole Y module, but on the smallest piece of Y that needs to do X? E.g.:

#[allow(usage_of_ty_tykind)]
pub use self::TyKind::*;

@Centril
Copy link
Contributor

Centril commented May 31, 2019

New internal lint idea:

  • ban identifiers did (also vid, ...) when they refer to DefID and similar.

cc @oli-obk #61350 (comment)

@flip1995
Copy link
Member

flip1995 commented Jun 13, 2019

Internal lint idea:

  • Check that error / lint / help / ... messages start lower case

This is a unwritten / hard to find rule of the rust compiler, which new contributors, especially in Clippy often get wrong (and is sometimes overlooked by the reviewers). Since rustc has a huge amount of functions that can produce error / lint / help / ... messages, this lint could be a bit tedious to implement.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 13, 2019

Moderately stupid idea: just panic in the Diagnostic type if a string argument starts with a capitalized character.

Centril added a commit to Centril/rust that referenced this issue Jul 5, 2019
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
Centril added a commit to Centril/rust that referenced this issue Jul 5, 2019
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
Centril added a commit to Centril/rust that referenced this issue Jul 5, 2019
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
@Centril
Copy link
Contributor

Centril commented Aug 1, 2019

cc #63186

@jonas-schievink jonas-schievink added the A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. label Apr 20, 2020
@jyn514 jyn514 added the A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself label Mar 8, 2021
@jyn514
Copy link
Member

jyn514 commented Mar 8, 2021

ban pub use foo::bar;, #65233

This issue was closed, I think it should be removed from the list.

calling clone on Copy types (#27266)

That issue was closed because there's a clippy lint for it - maybe bootstrap should just pass -W clippy::clone_on_copy?

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 8, 2021

That issue was closed because there's a clippy lint for it - maybe bootstrap should just pass -W clippy::clone_on_copy?

bootstrap doesn't run with clippy yet. I guess we could replace ./x.py check with a clippy run with the beta clippy?

@jyn514
Copy link
Member

jyn514 commented Mar 8, 2021

That issue was closed because there's a clippy lint for it - maybe bootstrap should just pass -W clippy::clone_on_copy?

bootstrap doesn't run with clippy yet. I guess we could replace ./x.py check with a clippy run with the beta clippy?

It works imperfectly since #77351. It shouldn't be terribly hard to make it use beta clippy, I just haven't gotten around to it (right now it uses the default clippy of your host toolchain).

@pnkfelix pnkfelix added the E-help-wanted Call for participation: Help is requested to fix this issue. label Jun 3, 2022
@jackh726 jackh726 added the S-tracking-impl-incomplete Status: The implementation is incomplete. label Jun 3, 2022
@oli-obk oli-obk added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Jun 13, 2022
@oli-obk oli-obk removed their assignment Jul 7, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 17, 2023
…li-obk

Implement an internal lint encouraging use of `Span::eq_ctxt`

Adds a new Rustc internal lint that forbids use of `.ctxt() == .ctxt()` for spans, encouraging use of `.eq_ctxt()` instead (see rust-lang#49509).

Also fixed a few violations of the lint in the Rustc codebase (a fun additional way I could test my code). Edit: MIR opt folks I believe that's why you're CC'ed, just a heads up.

Two things I'm not sure about:
1. The way I chose to detect calls to `Span::ctxt`. I know adding diagnostic items to methods is generally discouraged, but after some searching and experimenting I couldn't find anything else that worked, so I went with it. That said, I'm happy to implement something different if there's a better way out there. (For what it's worth, if there is a better way, it might be worth documenting in the rustc-dev-guide, which I'm happy to take care of)
2. The error message for the lint. Ideally it would probably be good to give some context as to why the suggestion is made (e.g. `rustc::default_hash_types` tells the user that it's because of performance), but I don't have that context so I couldn't put it in the error message. Happy to iterate on the error message based on feedback during review.

r? `@oli-obk`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 17, 2023
…li-obk

Implement an internal lint encouraging use of `Span::eq_ctxt`

Adds a new Rustc internal lint that forbids use of `.ctxt() == .ctxt()` for spans, encouraging use of `.eq_ctxt()` instead (see rust-lang#49509).

Also fixed a few violations of the lint in the Rustc codebase (a fun additional way I could test my code). Edit: MIR opt folks I believe that's why you're CC'ed, just a heads up.

Two things I'm not sure about:
1. The way I chose to detect calls to `Span::ctxt`. I know adding diagnostic items to methods is generally discouraged, but after some searching and experimenting I couldn't find anything else that worked, so I went with it. That said, I'm happy to implement something different if there's a better way out there. (For what it's worth, if there is a better way, it might be worth documenting in the rustc-dev-guide, which I'm happy to take care of)
2. The error message for the lint. Ideally it would probably be good to give some context as to why the suggestion is made (e.g. `rustc::default_hash_types` tells the user that it's because of performance), but I don't have that context so I couldn't put it in the error message. Happy to iterate on the error message based on feedback during review.

r? ``@oli-obk``
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 17, 2023
Rollup merge of rust-lang#116787 - a-lafrance:span-internal-lint, r=oli-obk

Implement an internal lint encouraging use of `Span::eq_ctxt`

Adds a new Rustc internal lint that forbids use of `.ctxt() == .ctxt()` for spans, encouraging use of `.eq_ctxt()` instead (see rust-lang#49509).

Also fixed a few violations of the lint in the Rustc codebase (a fun additional way I could test my code). Edit: MIR opt folks I believe that's why you're CC'ed, just a heads up.

Two things I'm not sure about:
1. The way I chose to detect calls to `Span::ctxt`. I know adding diagnostic items to methods is generally discouraged, but after some searching and experimenting I couldn't find anything else that worked, so I went with it. That said, I'm happy to implement something different if there's a better way out there. (For what it's worth, if there is a better way, it might be worth documenting in the rustc-dev-guide, which I'm happy to take care of)
2. The error message for the lint. Ideally it would probably be good to give some context as to why the suggestion is made (e.g. `rustc::default_hash_types` tells the user that it's because of performance), but I don't have that context so I couldn't put it in the error message. Happy to iterate on the error message based on feedback during review.

r? ``@oli-obk``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. S-tracking-impl-incomplete Status: The implementation is incomplete. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests