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.

Sign up for GitHub

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

Jump to bottom

force-warn for edition lints #85512

Closed
4 tasks done
nikomatsakis opened this issue May 20, 2021 · 9 comments
Closed
4 tasks done

force-warn for edition lints #85512

nikomatsakis opened this issue May 20, 2021 · 9 comments
Assignees
@rylev
Labels
A-edition-2021 Area: The 2021 edition A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 20, 2021 •
edited
Loading

Summary

Compiler changes:

  • Create MCP
  • Add a --force-warn XXX option that is given either a lint or lint group XXX
  • When this command line option is used, all other lint settings the lints covered by XXX will always issue warnings, regardless of whether they are "allowed" by the code or command-line options

Cargo fix changes:

  • This command line option is supplied by cargo fix --edition in order to force migration lints etc to be considered

Motivation

We would like to take some existing warnings and make them into errors in the new edition. We anticipate this being a common pattern. The problem is that these warnings, if they already exist, may be marked a #[allow] in various code bases. As a result, cargo fix would not see the migration suggestions and migration would not succeed.

Proposed plan

To become part of a migration, existing lints can be directly added into rust-2021-migration group.

Caveat: Multiple groups

This plan means that some lints are members of multiple groups. This has been discouraged but in the past but we currently believe that it should work fine. We should test the scenario where a lint is in two groups and one of those groups is allow.

Alternatives

We could instead introduce a fresh copy of these lints that is a member of rust_2021_migrations. For example, if there is a lint foo, maybe we make a foo_2021 lint that is specifically for the migration. We could perhaps make this convenient to issue in the code by having some option when creating the lint that is like .migration(RUST_2021_MIGRATioN). This could also make the lint into a hard error automatically in the new edition, regardless of the lint level.
@nikomatsakis
Copy link
Contributor Author

@rustbot assign @rylev

@nikomatsakis nikomatsakis added A-edition-2021 Area: The 2021 edition T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 20, 2021
@nikomatsakis
Copy link
Contributor Author

Some mentoring notes:

  • To add an option, modify this code to add the force-warn, modeled on cap-lints:

https://github.com/rust-lang/rust/blob/0eb4e811915eae2964e6072086d4a13c8daf136b/compiler/rustc_session/src/options.rs#L131-L133

Extend the LintLevelMap to contain a list of "force warn" lists:

https://github.com/rust-lang/rust/blob/0eb4e811915eae2964e6072086d4a13c8daf136b/compiler/rustc_middle/src/lint.rs#L152-L157

Modify the lint_levels query to populate the list from the command line option:

https://github.com/rust-lang/rust/blob/0eb4e811915eae2964e6072086d4a13c8daf136b/compiler/rustc_lint/src/levels.rs#L31-L32

Extend LintLevelSource with an option for ForceWarn:

https://github.com/rust-lang/rust/blob/0eb4e811915eae2964e6072086d4a13c8daf136b/compiler/rustc_middle/src/lint.rs#L17-L32

Modify the level_and_source to check the list "force warn" and return Warn if the lint is present, regardless of the other parameters (return ForceWarn):

https://github.com/rust-lang/rust/blob/0eb4e811915eae2964e6072086d4a13c8daf136b/compiler/rustc_middle/src/lint.rs#L166-L173

@nikomatsakis
Copy link
Contributor Author

Write some tests:

  • // compile-flags: --force-warn XXX to add flags
  • Test for:
    • Force warn on a lint name that is allowed by name
    • Force warn on a lint name that is allowed by a group that contains it
    • Force warn on a lint group that contains a lint allowed by its name
    • Force warn on a lint group that contains a lint allowed by group
    • Force warn on a lint group that contains a lint allowed by some other group
    • Force warn on a lint that is allowed by warnings group (#[allow(warnings)])

@jam1garner jam1garner mentioned this issue May 26, 2021
Merged
6 tasks
@jam1garner
Copy link
Contributor

We could perhaps make this convenient to issue in the code by having some option when creating the lint that is like .migration(RUST_2021_MIGRATioN). This could also make the lint into a hard error automatically in the new edition, regardless of the lint level.

I was under the impression that most (if not all) migrations lints will already result in an error by means of the fact that the code will break. For example with the TryFrom/TryInto/FromIterator migration lint, even if the lint is set as a warning/error on 2021 edition, it would never be hit due to hitting error[E0034]: multiple applicable items in scope and compilation stopping before the lint (as the lint has to occur after trait resolution to know whether the trait is actually outside of std/core).

If the purpose of such is for users to just change edition to 2021 without running migration fixes to get an error indicating the error being 2021-edition-related, I would imagine we'd need to implement an additional bit of detection/help text for that emission of E0034.

Such a help text should be possible, just detecting that one of the traits in the set of applicable items is TryFrom/TryInto/FromIterator should be enough. Although we should likely also detect that the import comes from being auto-imported, or if we don't make it clear in the help text that it only "might" be 2021-related.

@nikomatsakis
Copy link
Contributor Author

@jam1garner that text was referring specifically to those warnings which will become hard errors in the new edition, eg the items in #83213

@rylev rylev mentioned this issue May 28, 2021
Merged
This was referenced Jun 1, 2021
Closed
Closed
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 4, 2021
@bors
Auto merge of rust-lang#85788 - rylev:force-warns, r=nikomatsakis
595088d 
Support for force-warns

Implements rust-lang#85512.

This PR adds a new command line option `force-warns` which will force the provided lints to warn even if they are allowed by some other mechanism such as `#![allow(warnings)]`.

Some remaining issues:
* rust-lang#85512 mentions that `force-warns` should also be capable of taking lint groups instead of individual lints. This is not implemented.
* If a lint has a higher warning level than `warn`, this will cause that lint to warn instead. We probably want to allow the lint to error if it is set to a higher lint and is not allowed somewhere else.
* One test is currently ignored because it's not working - when a deny-by-default lint is allowed, it does not currently warn under `force-warns`. I'm not sure why, but I wanted to get this in before the weekend.

r? `@nikomatsakis`
@inquisitivecrystal
Copy link
Contributor

@rustbot label +A-lint

@rustbot rustbot added the A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. label Jun 5, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Jun 7, 2021

Now that #85788 is merged, what steps are left?

@ehuss
Copy link
Contributor

ehuss commented Jun 7, 2021

I'll post a Cargo PR to start using the new flag, and enable 2021 migrations.

There is some follow-up working being pursued (#86009 and supporting --cap-lints), that I think would be good to resolve before stabilizing the flag. (And it needs to be stabilized before 2021 is stabilized.)

@rylev
Copy link
Member

rylev commented Jun 9, 2021

I'll work on stabilization. 👍

@nikomatsakis nikomatsakis mentioned this issue Jun 21, 2021
Closed
fee1-dead added a commit to fee1-dead-contrib/rust that referenced this issue Jun 22, 2021
@fee1-dead
Rollup merge of rust-lang#85707 - jam1garner:future_prelude_collision…
4c27d8e 
…_lint, r=nikomatsakis

Add `future_prelude_collision` lint

Implements rust-lang#84594. (RFC rust-lang/rfcs#3114 ([rendered](https://github.com/rust-lang/rfcs/blob/master/text/3114-prelude-2021.md))) Not entirely complete but wanted to have my progress decently available while I finish off the last little bits.

Things left to implement:

* [x] UI tests for lints
* [x] Only emit lint for 2015 and 2018 editions
* [ ] Lint name/message bikeshedding
* [x] Implement for `FromIterator` (from best I can tell, the current approach as mentioned from [this comment](rust-lang#84594 (comment)) won't work due to `FromIterator` instances not using dot-call syntax, but if I'm correct about this then that would also need to be fixed for `TryFrom`/`TryInto`)*
* [x] Add to `rust-2021-migration` group? (See rust-lang#85512) (added to `rust-2021-compatibility` group)
* [ ] Link to edition guide in lint docs

*edit: looked into it, `lookup_method` will also not be hit for `TryFrom`/`TryInto` for non-dotcall syntax. If anyone who is more familiar with typecheck knows the equivalent for looking up associated functions, feel free to chime in.
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 22, 2021
@bors
Auto merge of rust-lang#85707 - jam1garner:future_prelude_collision_l…
44f4a87 
…int, r=nikomatsakis

Add `future_prelude_collision` lint

Implements rust-lang#84594. (RFC rust-lang/rfcs#3114 ([rendered](https://github.com/rust-lang/rfcs/blob/master/text/3114-prelude-2021.md))) Not entirely complete but wanted to have my progress decently available while I finish off the last little bits.

Things left to implement:

* [x] UI tests for lints
* [x] Only emit lint for 2015 and 2018 editions
* [ ] Lint name/message bikeshedding
* [x] Implement for `FromIterator` (from best I can tell, the current approach as mentioned from [this comment](rust-lang#84594 (comment)) won't work due to `FromIterator` instances not using dot-call syntax, but if I'm correct about this then that would also need to be fixed for `TryFrom`/`TryInto`)*
* [x] Add to `rust-2021-migration` group? (See rust-lang#85512) (added to `rust-2021-compatibility` group)
* [ ] Link to edition guide in lint docs

*edit: looked into it, `lookup_method` will also not be hit for `TryFrom`/`TryInto` for non-dotcall syntax. If anyone who is more familiar with typecheck knows the equivalent for looking up associated functions, feel free to chime in.
@rylev rylev mentioned this issue Jun 23, 2021
Merged
@m-ou-se m-ou-se closed this as completed Jun 28, 2021
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 6, 2021
@bors
Auto merge of rust-lang#86572 - rylev:force-warnings-always, r=nikoma…
238fd72 
…tsakis

Force warnings even when can_emit_warnings == false

Fixes an issue mentioned in rust-lang#85512 with --cap-lints overriding --force-warnings.

Fixes rust-lang#86751

r? `@ehuss`
@xFrednet xFrednet mentioned this issue Mar 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rylev rylev

Labels
A-edition-2021 Area: The 2021 edition A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@ehuss @nikomatsakis @m-ou-se @rylev @jam1garner @inquisitivecrystal @rustbot