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

rustc_session: allow overriding lint level of individual lints from a group #67885

Merged
merged 4 commits into from
Feb 16, 2020

Conversation

tobithiel
Copy link
Contributor

@tobithiel tobithiel commented Jan 5, 2020

Fixes #58211 and fixes rust-lang/rust-clippy#4778 and fixes rust-lang/rust-clippy#4091

Instead of hard-coding the lint level preferences (from lowest to highest precedence: lint::Allow -> lint::Warn -> lint::Deny -> lint::Forbid), the position of the argument in the command line gets taken into account.

Examples:

  1. Passing -D unused -A unused-variables denies everything in the lint group unused except unused-variables which is explicitly allowed.
  2. Passing -A unused-variables -D unused denies everything in the lint group unused including unused-variables since the allow is specified before the deny (and therefore overridden by the deny).

This matches the behavior that is already being used when specifying allow/deny in the source code.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 5, 2020
@rust-highfive

This comment has been minimized.

@matthewjasper
Copy link
Contributor

cc @rust-lang/compiler for opinions on this change
cc @Mark-Simulacrum for the compiletest changes

@Mark-Simulacrum
Copy link
Member

compiletest changes look fine to me; I guess since CI is passing we weren't relying on the unused to override custom flags (which is good! :)

I personally think this change also makes sense in general -- I think I've wanted something like it in the past, even. We should make sure that Clippy folks are on board (cc @Manishearth -- do you know who all to ping inside dev tools?)

@petrochenkov
Copy link
Contributor

petrochenkov commented Jan 11, 2020

I'm generally for command line options being overridable by later command line options, because the need to set them hierarchically like #58211 describes arises in various contexts.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 12, 2020

cc @rust-lang/wg-diagnostics @rust-lang/clippy

@Manishearth
Copy link
Member

Clippy is happy with this we've wanted this to work well forever 😄

@Mark-Simulacrum
Copy link
Member

Could someone from the compiler team kick off FCP here? Even though no objections have been raised yet, and indeed everyone seems enthusiastically on board, seems like we should do so as a stable API change.

@oli-obk oli-obk added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 12, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Jan 12, 2020

This change unifies the behaviour between the command line lint level flags and the in source lint level attributes in the presence of conflicting flags. The last flag is now always the one that makes the decision, overriding previous flags.

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Jan 12, 2020

Team member @oli-obk has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 12, 2020
@nikomatsakis
Copy link
Contributor

Seems reasonable to me, but I would ask that we document it in the rustc book -- @tobithiel can you update https://github.com/rust-lang/rust/blob/master/src/doc/rustc/src/command-line-arguments.md to explain this behavior? (And perhaps add suitable note into https://github.com/rust-lang/rust/blob/master/src/doc/rustc/src/lints/index.md or elsewhere as you see fit)

@tobithiel
Copy link
Contributor Author

@nikomatsakis I added some examples to the documentation where it seemed to fit well. Let me know if you had other locations in mind as well. I'm not very familiar with the different documentations.

@nikomatsakis
Copy link
Contributor

Thanks @tobithiel, that looks great.

@matthewjasper matthewjasper added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 25, 2020
@goffrie
Copy link
Contributor

goffrie commented Feb 3, 2020

ping @Zoxc @estebank @nagisa @pnkfelix in case you missed this :)

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 6, 2020
@rfcbot
Copy link

rfcbot commented Feb 6, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot
Copy link

rfcbot commented Feb 16, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Feb 16, 2020
@Mark-Simulacrum Mark-Simulacrum added the relnotes Marks issues that should be documented in the release notes of the next release. label Feb 16, 2020
@Mark-Simulacrum
Copy link
Member

@bors r+

Let's merge this in! I've double checked the implementation and it looks good to me. Also tagging as relnotes, though this change will not be observable to most Rust users (as Cargo hides rustc's CLI by default).

@bors
Copy link
Contributor

bors commented Feb 16, 2020

📌 Commit 3fc9253 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Feb 16, 2020
@bors
Copy link
Contributor

bors commented Feb 16, 2020

⌛ Testing commit 3fc9253 with merge 5e7af46...

bors added a commit that referenced this pull request Feb 16, 2020
…k-Simulacrum

rustc_session: allow overriding lint level of individual lints from a group

Fixes #58211 and fixes rust-lang/rust-clippy#4778 and fixes rust-lang/rust-clippy#4091

Instead of hard-coding the lint level preferences (from lowest to highest precedence: `lint::Allow -> lint::Warn -> lint::Deny -> lint::Forbid`), the position of the argument in the command line gets taken into account.

Examples:
1. Passing `-D unused -A unused-variables` denies everything in the lint group `unused` **except** `unused-variables` which is explicitly allowed.
1. Passing `-A unused-variables -D unused` denies everything in the lint group `unused` **including** `unused-variables` since the allow is specified before the deny (and therefore overridden by the deny).

This matches the behavior that is already being used when specifying `allow`/`deny` in the source code.
@bors
Copy link
Contributor

bors commented Feb 16, 2020

☀️ Test successful - checks-azure
Approved by: Mark-Simulacrum
Pushing 5e7af46 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 16, 2020
@bors bors merged commit 3fc9253 into rust-lang:master Feb 16, 2020
@tobithiel tobithiel deleted the fix_group_lint_allow_override branch February 17, 2020 06:02
@RalfJung
Copy link
Member

RalfJung commented Apr 5, 2020

I think this PR caused the regression in #70819.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request May 16, 2020
Pkgsrc changes:
 * Bump rust bootstrap version to 1.42.0, except for Darwin/i686 where the
   bootstrap is not (yet?) available.

Upstream changes:

Version 1.43.0 (2020-04-23)
==========================

Language
--------
- [Fixed using binary operations with `&{number}` (e.g. `&1.0`) not having
  the type inferred correctly.][68129]
- [Attributes such as `#[cfg()]` can now be used on `if` expressions.][69201]

**Syntax only changes**
- [Allow `type Foo: Ord` syntactically.][69361]
- [Fuse associated and extern items up to defaultness.][69194]
- [Syntactically allow `self` in all `fn` contexts.][68764]
- [Merge `fn` syntax + cleanup item parsing.][68728]
- [`item` macro fragments can be interpolated into `trait`s, `impl`s,
  and `extern` blocks.][69366]
  For example, you may now write:
  ```rust
  macro_rules! mac_trait {
      ($i:item) => {
          trait T { $i }
      }
  }
  mac_trait! {
      fn foo() {}
  }
  ```
These are still rejected *semantically*, so you will likely receive an error but
these changes can be seen and parsed by macros and
conditional compilation.


Compiler
--------
- [You can now pass multiple lint flags to rustc to override the previous
  flags.][67885] For example; `rustc -D unused -A unused-variables` denies
  everything in the `unused` lint group except `unused-variables` which
  is explicitly allowed. However, passing `rustc -A unused-variables -D unused` denies
  everything in the `unused` lint group **including** `unused-variables` since
  the allow flag is specified before the deny flag (and therefore overridden).
- [rustc will now prefer your system MinGW libraries over its bundled libraries
  if they are available on `windows-gnu`.][67429]
- [rustc now buffers errors/warnings printed in JSON.][69227]

Libraries
---------
- [`Arc<[T; N]>`, `Box<[T; N]>`, and `Rc<[T; N]>`, now implement
  `TryFrom<Arc<[T]>>`,`TryFrom<Box<[T]>>`, and `TryFrom<Rc<[T]>>`
  respectively.][69538] **Note** These conversions are only available when `N`
  is `0..=32`.
- [You can now use associated constants on floats and integers directly, rather
  than having to import the module.][68952] e.g. You can now write `u32::MAX` or
  `f32::NAN` with no imports.
- [`u8::is_ascii` is now `const`.][68984]
- [`String` now implements `AsMut<str>`.][68742]
- [Added the `primitive` module to `std` and `core`.][67637] This module
  reexports Rust's primitive types. This is mainly useful in macros
  where you want avoid these types being shadowed.
- [Relaxed some of the trait bounds on `HashMap` and `HashSet`.][67642]
- [`string::FromUtf8Error` now implements `Clone + Eq`.][68738]

Stabilized APIs
---------------
- [`Once::is_completed`]
- [`f32::LOG10_2`]
- [`f32::LOG2_10`]
- [`f64::LOG10_2`]
- [`f64::LOG2_10`]
- [`iter::once_with`]

Cargo
-----
- [You can now set config `[profile]`s in your `.cargo/config`, or through
  your environment.][cargo/7823]
- [Cargo will now set `CARGO_BIN_EXE_<name>` pointing to a binary's
  executable path when running integration tests or benchmarks.][cargo/7697]
  `<name>` is the name of your binary as-is e.g. If you wanted the executable
  path for a binary named `my-program`you would use
  `env!("CARGO_BIN_EXE_my-program")`.

Misc
----
- [Certain checks in the `const_err` lint were deemed unrelated to const
  evaluation][69185], and have been moved to the `unconditional_panic` and
  `arithmetic_overflow` lints.

Compatibility Notes
-------------------

- [Having trailing syntax in the `assert!` macro is now a hard error.][69548]
  This has been a warning since 1.36.0.
- [Fixed `Self` not having the correctly inferred type.][69340] This incorrectly
  led to some instances being accepted, and now correctly emits a hard error.

[69340]: rust-lang/rust#69340

Internal Only
-------------
These changes provide no direct user facing benefits, but represent significant
improvements to the internals and overall performance of `rustc` and
related tools.

- [All components are now built with `opt-level=3` instead of `2`.][67878]
- [Improved how rustc generates drop code.][67332]
- [Improved performance from `#[inline]`-ing certain hot functions.][69256]
- [traits: preallocate 2 Vecs of known initial size][69022]
- [Avoid exponential behaviour when relating types][68772]
- [Skip `Drop` terminators for enum variants without drop glue][68943]
- [Improve performance of coherence checks][68966]
- [Deduplicate types in the generator witness][68672]
- [Invert control in struct_lint_level.][68725]

[67332]: rust-lang/rust#67332
[67429]: rust-lang/rust#67429
[67637]: rust-lang/rust#67637
[67642]: rust-lang/rust#67642
[67878]: rust-lang/rust#67878
[67885]: rust-lang/rust#67885
[68129]: rust-lang/rust#68129
[68672]: rust-lang/rust#68672
[68725]: rust-lang/rust#68725
[68728]: rust-lang/rust#68728
[68738]: rust-lang/rust#68738
[68742]: rust-lang/rust#68742
[68764]: rust-lang/rust#68764
[68772]: rust-lang/rust#68772
[68943]: rust-lang/rust#68943
[68952]: rust-lang/rust#68952
[68966]: rust-lang/rust#68966
[68984]: rust-lang/rust#68984
[69022]: rust-lang/rust#69022
[69185]: rust-lang/rust#69185
[69194]: rust-lang/rust#69194
[69201]: rust-lang/rust#69201
[69227]: rust-lang/rust#69227
[69548]: rust-lang/rust#69548
[69256]: rust-lang/rust#69256
[69361]: rust-lang/rust#69361
[69366]: rust-lang/rust#69366
[69538]: rust-lang/rust#69538
[cargo/7823]: rust-lang/cargo#7823
[cargo/7697]: rust-lang/cargo#7697
[`Once::is_completed`]: https://doc.rust-lang.org/std/sync/struct.Once.html#method.is_completed
[`f32::LOG10_2`]: https://doc.rust-lang.org/std/f32/consts/constant.LOG10_2.html
[`f32::LOG2_10`]: https://doc.rust-lang.org/std/f32/consts/constant.LOG2_10.html
[`f64::LOG10_2`]: https://doc.rust-lang.org/std/f64/consts/constant.LOG10_2.html
[`f64::LOG2_10`]: https://doc.rust-lang.org/std/f64/consts/constant.LOG2_10.html
[`iter::once_with`]: https://doc.rust-lang.org/std/iter/fn.once_with.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet