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

Promote Certain Lints to Errors in Rust 2021 Edition #80165

Closed
2 tasks done
m-ou-se opened this issue Dec 18, 2020 · 47 comments
Closed
2 tasks done

Promote Certain Lints to Errors in Rust 2021 Edition #80165

m-ou-se opened this issue Dec 18, 2020 · 47 comments
Assignees
Labels
A-edition-2018-lints Area: Lints supporting the 2018 edition A-edition-2021 Area: The 2021 edition C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-lang Relevant to the language team, which will review and decide on the PR/issue.
Milestone

Comments

@m-ou-se
Copy link
Member

m-ou-se commented Dec 18, 2020

Tracking issue for turning lints into errors in Rust 2021.

  • BARE_TRAIT_OBJECTS
  • ELLIPSIS_INCLUSIVE_RANGE_PATTERNS

cc @rylev

@m-ou-se m-ou-se added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC A-edition-2018-lints Area: Lints supporting the 2018 edition labels Dec 18, 2020
@m-ou-se m-ou-se added this to the Rust 2021 milestone Dec 18, 2020
@camelid
Copy link
Member

camelid commented Dec 18, 2020

  • MACRO_USE_EXTERN_CRATE

I'm not sure if this one should be deny-by-default; this is what the lint's page says:

This lint is "allow" by default because this is a stylistic choice that has not been settled, see issue #52043 for more information.

@camelid
Copy link
Member

camelid commented Dec 18, 2020

Also, MACRO_USE_EXTERN_CRATE is not warn-by-default – it's actually allow-by-default.

@m-ou-se
Copy link
Member Author

m-ou-se commented Dec 18, 2020

@rylev GitHub won't let me assign you to this issue; you wanted to work on this, right? I think you can @​rustbot claim to claim the issue.

@camelid
Copy link
Member

camelid commented Dec 18, 2020

@rustbot assign @rylev

@rustbot

This comment has been minimized.

@rustbot rustbot self-assigned this Dec 18, 2020
@camelid
Copy link
Member

camelid commented Dec 18, 2020

Hmm, I wonder why GitHub won't let us assign rylev directly – they are an org member...

@rylev
Copy link
Member

rylev commented Dec 18, 2020

@rustbot claim

@rustbot rustbot assigned rylev and unassigned rustbot Dec 18, 2020
@taiki-e
Copy link
Member

taiki-e commented Dec 19, 2020

  • UNUSED_EXTERN_CRATES
  • EXPLICIT_OUTLIVES_REQUIREMENTS
  • UNREACHABLE_PUB
  • MACRO_USE_EXTERN_CRATE

I think these lints belong to a similar category as unused lint. I think it probably makes sense to warn by default, but I don't think we need to deny it by default.

  • ELLIPSIS_INCLUSIVE_RANGE_PATTERNS
  • ELIDED_LIFETIMES_IN_PATHS

The other two (ELLIPSIS_INCLUSIVE_RANGE_PATTERNS and ELIDED_LIFETIMES_IN_PATHS) are related to compatibility or deprecation, and it seems reasonable to deny them in the next edition. (ELIDED_LIFETIMES_IN_PATHS is actually already rejected on async functions.)

@jyn514
Copy link
Member

jyn514 commented Dec 19, 2020

unreachable_pub still has bugs after three years: #64762. I don't think it should be even warn-by-default until the false positives are fixed.

@oli-obk

This comment has been minimized.

@RalfJung

This comment has been minimized.

@oli-obk

This comment has been minimized.

@RalfJung

This comment has been minimized.

@rylev
Copy link
Member

rylev commented Dec 30, 2020

Where we are now:

Promote to deny:

  • ELLIPSIS_INCLUSIVE_RANGE_PATTERNS
  • ELIDED_LIFETIMES_IN_PATHS

Do the above need discussion, or are the uncontroversial?

Needs more discussion:

  • MACRO_USE_EXTERN_CRATE: this is currently allow by default. More discussion needs to happen (likely in edition lint: migrating extern crate with #[macro_use] #52043) on whether this should be promoted to warn. This is likely to fall in the same category as other "unused" lints and thus might not be subject to an edition change.
  • BARE_TRAIT_OBJECTS: this is currently warn-by-default but it has not been settled whether this will be deny-by-default. There is currently discussion happening in rust-lang/lang-team#65
  • EXPLICIT_OUTLIVES_REQUIREMENTS and UNUSED_EXTERN_CRATES: Should this be promoted to warn, and do we need to do this on an edition boundary?

Needs implementation work:

  • UNREACHABLE_PUB: this lint gives false positives and so is not currently ready to be promoted.

@camelid
Copy link
Member

camelid commented Jan 6, 2021

There's also the mutable_borrow_reservation_conflict lint. There's a PR to turn it from warn-by-default to deny-by-default: #76104.

@nikomatsakis nikomatsakis added T-lang Relevant to the language team, which will review and decide on the PR/issue. I-nominated labels Jan 15, 2021
@nikomatsakis
Copy link
Contributor

I'm nominated this for @rust-lang/lang discussion. I'd like us to review these lints and make sure we are happy with them.

@m-ou-se
Copy link
Member Author

m-ou-se commented Jan 19, 2021

As discussed in the lang meeting: Let's have separate PRs to turn each of these lints into deny-by-default on 2021 (which should be trivial changes), such that each lint can have its down discussion on its own PR. Then we can use this issue as the tracking issue for tracking those PRs. (cc @rylev)

@joshtriplett
Copy link
Member

joshtriplett commented Jan 19, 2021

👍

We should also discuss (on an appropriate issue once filed) whether bare_trait_objects should be a deny-by-default lint, or a hard error. The argument for the latter is in rust-lang/lang-team#65 : making bare_trait_objects a hard error in an edition will make it possible to give better error messages for const-generics in that edition and newer.

@rylev
Copy link
Member

rylev commented Jan 20, 2021

I will submit PRs for each of these lints. 👍

@RalfJung
Copy link
Member

RalfJung commented Mar 3, 2021

This makes them practically ineligible for promotion to errors, but it can still be reasonable to promote them at edition boundary.

I agree. There are bugfix lints for which the question isn't "make them a hard error in all code or in new code"; the question is "make them a hard error in new code or not at all".

@Nemo157
Copy link
Member

Nemo157 commented Mar 3, 2021

I also just had this idea of making future-incompat lints into hard errors at the edition boundary in #82702. It seems like this is something that should be applied to all future-incompat lints at every edition boundary, independent of whether the warning may be promoted into a hard error in previous editions as well in the future.

No maintained code should ever have a future-incompat warning triggering since that code may stop compiling at any point in time, if users are going through the effort of migrating the code to the new edition while keeping code with a future-incompat warning around it seems like there is something going very wrong somewhere.

@Manishearth
Copy link
Member

I also just had this idea of making future-incompat lints into hard errors at the edition boundary in #82702. It seems like this is something that should be applied to all future-incompat lints at every edition boundary, independent of whether the warning may be promoted into a hard error in previous editions as well in the future.

Yes, with the caveat that not all future incompat lints can be automatically fixed with cargo fix

@rylev
Copy link
Member

rylev commented Mar 4, 2021

There are bugfix lints for which the question isn't "make them a hard error in all code or in new code"; the question is "make them a hard error in new code or not at all".

This is definitely something we should discuss. I don't think we had a frank discussion about future incompatibility lints that are so pervasive that changing them in older code would be too damaging to be practical.

Yes, with the caveat that not all future incompat lints can be automatically fixed with cargo fix

I'm not sure if you're implying that we can only make future imcompat lints errors on edition boundaries if they can be automatically fixed with rustfix, but if so, that's not the case. We don't guarantee that edition changes have to be 100% migratable with rustfix.

@Manishearth
Copy link
Member

I'm not sure if you're implying that we can only make future imcompat lints errors on edition boundaries if they can be automatically fixed with rustfix, but if so, that's not the case. We don't guarantee that edition changes have to be 100% migratable with rustfix.

I am not implying this. However, I am implying that edition breakages need to have a proper analysis of what they break, whether those breakages need to be autofixed, and if so, how they will be autofixed. This is something we're working on clarifying in the 2021 edition RFC.

Which means that we cannot have a blanket policy of upgrading future incompat lints to edition breakages since this manual analysis must be done as well.

@Aloso
Copy link
Contributor

Aloso commented Mar 5, 2021

I wouldn't want a lint to have to spend a whole edition as deny-by-default before it could be a hard error.

The problem is that deny-by-default lints can be #[allow]ed. If someone has allowed bare trait objects and then upgrades to an edition that makes them deny-by-default, their code doesn't break. However, if a new release or edition turns it into a hard error, it's a breaking change (with no prior warning, since the warning was silenced).

One possible remedy is to issue a warning when someone allows a deny-by-default lint that is supposed to become a hard error in a future release.

For the person that has allowed bare_trait_objects, this means:

  • If they migrate to edition_2021, they get a new warning: "bare trait objects will become a hard error in a future release in the 2021 edition. Please update your code to use the dyn Trait syntax and remove the #[allow(bare_trait_objects)] attribute"
  • If they stay on edition_2018, their code will continue to work
  • If they migrate to edition_2021 after bare_trait_objects has been made a hard error in the 2021 edition, they get errors, even though their code built without warnings before. But this seems acceptable to me.

@Manishearth
Copy link
Member

Manishearth commented Mar 5, 2021

Okay so after going through this discussion I feel like we are retreading a lot of the work done for migration/idiom lints in the last edition, and facing the same dillemmas. We already decided on a lot of this in 2018.

There's also some discussion here that seems to redecide lint policy that has been so far never tied to editions.

If I recall correctly:

  • Idiom lints are "allow in edition X, warn in edition X + 1"
    • It is acceptable for idiom lints to be "allow in edition X, warn in X+1, deny in X+1 after a period of time" (we never implemented this)
  • Migration lints are "allow/warn in edition X, warn when migrating to X+1, hard error after migrating to X+1"
  • Future incompat lints can and often should be turned into migration lints (i.e. they become hard errors in that edition). Note that the edition allows for entirely new lints to be immediately made into a hard error, so this is, if anything, a weaker change.
  • Idiom lints from previous editions probably should be made into migration lints in the next edition (i.e. they become a hard error). bare_trait_objects is one of these
  • It is not a big deal to turn any lint, allow or warn, into an edition migration lint.
  • It is not a big deal to turn any lint, allow or warn, into deny by default, at any point in time, though we should be careful about blast radius.
  • The edition upgrade mechanism should itself include a lint for allow() on any lints that have been turned into migration lints, if we ever do this

For turning something into a migration lint I really do not see a need to go "one step at a time". The edition involves entirely new things -- never before linted -- being made into hard errors, it should be even less of a problem for things which have been linted for ages.

I'll point out, bare_trait_objects is an idiom lint from 2018, and there was a general feeling that it would probably be a migration lint (warn -> hard error) in 2021.


There is the future incompatible warning that explicitly states that a warning will become a hard error in a future edition. Is this a requirement before upgrading a warn-by-default lint to deny-by-default? Or is it acceptable for a lint to be promoted when it was not explicitly stated that promotion would happen?

There is already a stability policy for lints. It is acceptable for lints to randomly change default lint levels without it ever being considered breaking, due to cap-lint-level. It is unacceptable for lints to turn into hard, nonsilenceable errors.

There is a strong distinction between deny-by-default and hard error that is being missed here. deny-by-default is silenceable, and does not break dependencies. On the other hand hard errors are not silenceable and always break dependencies. New hard errors for things which previously compiled or linted are always breaking changes, new deny lints for things which previously compiled or warn-linted are never breaking changes.

Turning a warn into a deny never need be tied to an edition, it can be for making people want to do it more. Turning a warn into a hard error almost always must be tied to an edition (or an argument should be made about why the breakage is acceptable and the blast radius should be surveyed -- this happens with future incompat stuff)

@camelid
Copy link
Member

camelid commented Mar 11, 2021

I'm not sure if we're including rustdoc lints in this list, but I wonder if it might be a good idea to promote rustdoc::broken_intra_doc_links and maybe rustdoc::private_intra_doc_links to deny-by-default lints.

@jyn514
Copy link
Member

jyn514 commented Mar 11, 2021

I don't think those should be denied by default. The documentation still works fine even if the links are broken, and if people care they can always deny the lint themselves.

@Manishearth
Copy link
Member

Manishearth commented Mar 11, 2021

I'm not sure if we're including rustdoc lints in this list, but I wonder if it might be a good idea to promote rustdoc::broken_intra_doc_links and maybe rustdoc::private_intra_doc_links to deny-by-default lints.

Please no, these are not particularly bad, and are sometimes possible to trigger by accident. The bar for deny lints in rustc is pretty high, basically "this is very bad or deprecated"

@nikomatsakis
Copy link
Contributor

Closing this as done! Thanks @rylev for pushing on this.

dtolnay added a commit to dtolnay/miniserde that referenced this issue Jun 5, 2021
    warning: trait objects without an explicit `dyn` are deprecated
      --> src/ignore.rs:48:12
       |
    48 |         Ok(Visitor::ignore())
       |            ^^^^^^^ help: use `dyn`: `<dyn Visitor>`
       |
       = note: `#[warn(bare_trait_objects)]` on by default
       = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in the 2021 edition!
       = note: for more information, see issue #80165 <rust-lang/rust#80165>

    warning: trait objects without an explicit `dyn` are deprecated
      --> src/ignore.rs:58:12
       |
    58 |         Ok(Visitor::ignore())
       |            ^^^^^^^ help: use `dyn`: `<dyn Visitor>`
       |
       = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in the 2021 edition!
       = note: for more information, see issue #80165 <rust-lang/rust#80165>
github-actions bot pushed a commit to rust-lang/glacier that referenced this issue Jun 13, 2021
=== stdout ===
=== stderr ===
warning: trait objects without an explicit `dyn` are deprecated
 --> /home/runner/work/glacier/glacier/ices/86132.rs:4:36
  |
4 | const TRAIT_OBJ_UNALIGNED_VTABLE: &Trait =
  |                                    ^^^^^ help: use `dyn`: `dyn Trait`
  |
  = note: `#[warn(bare_trait_objects)]` on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in the 2021 edition!
  = note: for more information, see issue #80165 <rust-lang/rust#80165>

warning: constant is never used: `TRAIT_OBJ_UNALIGNED_VTABLE`
 --> /home/runner/work/glacier/glacier/ices/86132.rs:4:1
  |
4 | / const TRAIT_OBJ_UNALIGNED_VTABLE: &Trait =
5 | |     unsafe { mem::transmute((&92u8, &[0b_______001_11i128; 128])) };
  | |____________________________________________________________________^
  |
  = note: `#[warn(dead_code)]` on by default

error: any use of this value will cause an error
 --> /home/runner/work/glacier/glacier/ices/86132.rs:5:14
  |
4 | / const TRAIT_OBJ_UNALIGNED_VTABLE: &Trait =
5 | |     unsafe { mem::transmute((&92u8, &[0b_______001_11i128; 128])) };
  | |______________^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^__-
  |                |
  |                invalid vtable: alignment `7` is not a power of 2
  |
  = note: `#[deny(const_err)]` on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #71800 <rust-lang/rust#71800>

error: aborting due to previous error; 2 warnings emitted

==============
github-actions bot pushed a commit to rust-lang/glacier that referenced this issue Jun 22, 2021
=== stdout ===
=== stderr ===
warning: trait objects without an explicit `dyn` are deprecated
 --> /home/runner/work/glacier/glacier/ices/83630.rs:2:14
  |
2 | static DATA: Iterator<Item = _> = "my string";
  |              ^^^^^^^^^^^^^^^^^^ help: use `dyn`: `dyn Iterator<Item = _>`
  |
  = note: `#[warn(bare_trait_objects)]` on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in the 2021 edition!
  = note: for more information, see issue #80165 <rust-lang/rust#80165>

error[E0121]: the type placeholder `_` is not allowed within types on item signatures
 --> /home/runner/work/glacier/glacier/ices/83630.rs:2:30
  |
2 | static DATA: Iterator<Item = _> = "my string";
  |                              ^ not allowed in type signatures

error: aborting due to previous error; 1 warning emitted

For more information about this error, try `rustc --explain E0121`.
==============
JohnTitor pushed a commit to rust-lang/glacier that referenced this issue Jun 22, 2021
=== stdout ===
=== stderr ===
warning: trait objects without an explicit `dyn` are deprecated
 --> /home/runner/work/glacier/glacier/ices/83630.rs:2:14
  |
2 | static DATA: Iterator<Item = _> = "my string";
  |              ^^^^^^^^^^^^^^^^^^ help: use `dyn`: `dyn Iterator<Item = _>`
  |
  = note: `#[warn(bare_trait_objects)]` on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in the 2021 edition!
  = note: for more information, see issue #80165 <rust-lang/rust#80165>

error[E0121]: the type placeholder `_` is not allowed within types on item signatures
 --> /home/runner/work/glacier/glacier/ices/83630.rs:2:30
  |
2 | static DATA: Iterator<Item = _> = "my string";
  |                              ^ not allowed in type signatures

error: aborting due to previous error; 1 warning emitted

For more information about this error, try `rustc --explain E0121`.
==============

Co-authored-by: rustbot <rustbot@users.noreply.github.com>
github-actions bot pushed a commit to rust-lang/glacier that referenced this issue Jul 4, 2021
=== stdout ===
=== stderr ===
error: lifetime in trait object type must be followed by `+`
 --> /home/runner/work/glacier/glacier/ices/85350.rs:1:26
  |
1 | impl FnMut(&Context) for 'tcx {
  |                          ^^^^

error[E0407]: method `print` is not a member of trait `FnMut`
 --> /home/runner/work/glacier/glacier/ices/85350.rs:2:5
  |
2 |     fn print () -> Self :: Output{ }
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not a member of trait `FnMut`

error[E0412]: cannot find type `Context` in this scope
 --> /home/runner/work/glacier/glacier/ices/85350.rs:1:13
  |
1 | impl FnMut(&Context) for 'tcx {
  |             ^^^^^^^ not found in this scope
  |
help: consider importing one of these items
  |
1 | use core::task::Context;
  |
1 | use std::task::Context;
  |

warning: trait objects without an explicit `dyn` are deprecated
 --> /home/runner/work/glacier/glacier/ices/85350.rs:1:26
  |
1 | impl FnMut(&Context) for 'tcx {
  |                          ^^^^ help: use `dyn`: `dyn 'tcx`
  |
  = note: `#[warn(bare_trait_objects)]` on by default
  = warning: this is accepted in the current edition (Rust 2018) but is a hard error in Rust 2021!
  = note: for more information, see issue #80165 <rust-lang/rust#80165>

error[E0601]: `main` function not found in crate `85350`
 --> /home/runner/work/glacier/glacier/ices/85350.rs:1:1
  |
1 | / impl FnMut(&Context) for 'tcx {
2 | |     fn print () -> Self :: Output{ }
3 | | }
  | |_^ consider adding a `main` function to `/home/runner/work/glacier/glacier/ices/85350.rs`

error[E0224]: at least one trait is required for an object type
 --> /home/runner/work/glacier/glacier/ices/85350.rs:1:26
  |
1 | impl FnMut(&Context) for 'tcx {
  |                          ^^^^

error[E0261]: use of undeclared lifetime name `'tcx`
 --> /home/runner/work/glacier/glacier/ices/85350.rs:1:26
  |
1 | impl FnMut(&Context) for 'tcx {
  |     -                    ^^^^ undeclared lifetime
  |     |
  |     help: consider introducing lifetime `'tcx` here: `<'tcx>`
  |
  = help: if you want to experiment with in-band lifetime bindings, add `#![feature(in_band_lifetimes)]` to the crate attributes

error[E0229]: associated type bindings are not allowed here
 --> /home/runner/work/glacier/glacier/ices/85350.rs:1:6
  |
1 | impl FnMut(&Context) for 'tcx {
  |      ^^^^^^^^^^^^^^^ associated type not allowed here

error[E0212]: cannot use the associated type of a trait with uninferred generic parameters
 --> /home/runner/work/glacier/glacier/ices/85350.rs:2:20
  |
2 |     fn print () -> Self :: Output{ }
  |                    ^^^^^^^^^^^^^^ help: use a fully qualified path with inferred lifetimes: `<[type error] as FnOnce<(&[type error],)>>::Output`

error: aborting due to 8 previous errors; 1 warning emitted

Some errors have detailed explanations: E0212, E0224, E0229, E0261, E0407, E0412, E0601.
For more information about an error, try `rustc --explain E0212`.
==============
github-actions bot pushed a commit to rust-lang/glacier that referenced this issue Jul 28, 2021
=== stdout ===
=== stderr ===
error[E0403]: the name `T` is already used for a generic parameter in this item's generic parameters
 --> /home/runner/work/glacier/glacier/ices/86756.rs:1:14
  |
1 | trait Foo<T, T = T> {}
  |           -  ^ already used
  |           |
  |           first use of `T`

warning: trait objects without an explicit `dyn` are deprecated
 --> /home/runner/work/glacier/glacier/ices/86756.rs:3:15
  |
3 |     eq::<dyn, Foo>
  |               ^^^ help: use `dyn`: `dyn Foo`
  |
  = note: `#[warn(bare_trait_objects)]` on by default
  = warning: this is accepted in the current edition (Rust 2018) but is a hard error in Rust 2021!
  = note: for more information, see issue #80165 <rust-lang/rust#80165>

error[E0601]: `main` function not found in crate `86756`
 --> /home/runner/work/glacier/glacier/ices/86756.rs:1:1
  |
1 | / trait Foo<T, T = T> {}
2 | | fn eq<A, B>() {
3 | |     eq::<dyn, Foo>
4 | | }
  | |_^ consider adding a `main` function to `/home/runner/work/glacier/glacier/ices/86756.rs`

error[E0224]: at least one trait is required for an object type
 --> /home/runner/work/glacier/glacier/ices/86756.rs:3:10
  |
3 |     eq::<dyn, Foo>
  |          ^^^

error[E0107]: missing generics for trait `Foo`
 --> /home/runner/work/glacier/glacier/ices/86756.rs:3:15
  |
3 |     eq::<dyn, Foo>
  |               ^^^ expected at least 1 generic argument
  |
note: trait defined here, with at least 1 generic parameter: `T`
 --> /home/runner/work/glacier/glacier/ices/86756.rs:1:7
  |
1 | trait Foo<T, T = T> {}
  |       ^^^ -
help: add missing generic argument
  |
3 |     eq::<dyn, Foo<T>>
  |               ^^^^^^

error: aborting due to 4 previous errors; 1 warning emitted

Some errors have detailed explanations: E0107, E0224, E0403, E0601.
For more information about an error, try `rustc --explain E0107`.
==============
@auula
Copy link

auula commented Jul 31, 2021

“dyn” this was previously accepted by the compiler but is being phased out; it will become a hard error in the 2021 edition!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2018-lints Area: Lints supporting the 2018 edition A-edition-2021 Area: The 2021 edition C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests