Add compiler-internal lints

oli-obk commented 6 years ago

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.

[x] forbid uses of HashSet or HashMap (suggest FxHash*)
[ ] suggest using TyCtxtAt over TyCtxt + Span as arguments
[ ] calling clone on Copy types (https://github.com/rust-lang/rust/issues/27266)
[ ] do not use features_untracked when a tcx is in scope
[x] do not use TyKind::Foo, use ty::Foo instead
[x] do not use TyKind as a type outside the sty module, use Ty instead
[ ] don't use TyS outside rustc::ty::sty
[ ] don't use Vec::new() use vec![]
[x] don't use == for span contexts, use eq_ctxt instead (https://github.com/rust-lang/rust/pull/116787)
[ ] prefer param_env_reveal_all_normalized over parram_env(did).with_reveal_all_normalized() (https://github.com/rust-lang/rust/pull/121387#issuecomment-1963666972)
[ ] require 'tcx to be the last lifetime in generic args of type decls, functions and impls (see #130294) https://github.com/rust-lang/rust/labels/E-easy
[ ] forbid storing a TyCtxt and a Session in the same struct or use both in function args. Only use tcx https://github.com/rust-lang/rust/labels/E-easy

leonardo-m commented 6 years ago

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 commented 6 years ago

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.

eddyb commented 5 years ago

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

oli-obk commented 5 years ago

cc https://github.com/rust-lang/rust-clippy/issues/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 commented 5 years ago

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 commented 5 years ago

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

flip1995 commented 5 years ago

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 commented 5 years ago

adding new lint ideas

don't use TyS outside rustc::ty
don't use Vec::new() use vec![] (cc @eddyb)

eddyb commented 5 years ago

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 commented 5 years ago

New internal lint idea:

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

cc @oli-obk https://github.com/rust-lang/rust/pull/61350#discussion_r289366939

flip1995 commented 5 years ago

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 commented 5 years ago

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

Centril commented 5 years ago

cc https://github.com/rust-lang/rust/issues/63186

jyn514 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/rust-lang/rust/pull/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).

rust-lang / rust

Add compiler-internal lints #49509