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

Should the rust_2018_idioms lint group be mentioned in documentation? #52679

Closed
alexcrichton opened this issue Jul 24, 2018 · 13 comments
Closed
Assignees
Labels
A-edition-2018-lints Area: Lints supporting the 2018 edition

Comments

@alexcrichton
Copy link
Member

Enabling the rust_2018_idioms lint group right now is a pretty brutal experience. Once enabled a slew of warnings show up that cargo fix often isn't very well to prepared. Furthermore, not all lints have automatic suggestions! This runs the risk of making the 2018 edition feel like a "big breaking change" because it takes so much effort to quelch all the lints.

Should we completely not mention the rust_2018_idioms lint group for the edition? For the preview?

I'd personally think that we should avoid talking about this lint group at all until it's received a lot more polish. Until that time it's probably best if it's left only for compiler developers and the extra intrepid who go poking around in history. Curious to hear what others think though!

@alexcrichton alexcrichton added the A-edition-2018-lints Area: Lints supporting the 2018 edition label Jul 24, 2018
@alexcrichton alexcrichton added this to the Rust 2018 Preview 2 milestone Jul 24, 2018
@aturon
Copy link
Member

aturon commented Jul 24, 2018

@Manishearth
Copy link
Member

IIRC one of the original plans was something like:

  • After upgrading you can optionally run cargo fix --idioms. This turns on these lints and fixes as many as possible. It won't complain about unfixable things (for now).
  • Six months into the edition we make these on by default on 2018 (or something like that)

@Centril
Copy link
Contributor

Centril commented Jul 25, 2018

I'd personally think that we should avoid talking about this lint group at all until it's received a lot more polish.

That seems reasonable; on the other hand, you'll get less testing on the lint group if you do that...
But I do think we should stick to the plan @Manishearth noted eventually (not necessarily enabling for preview 2).

@zackmdavis
Copy link
Member

I think it's worth distinguishing what behavior we want from something like cargo fix --idioms from what documentation should say about #[warn(rust_2018_idioms)].

I think the bare-trait-objects and elided-lifetimes-in-paths lints should Just Work (absent macro shenanigans) and ellipsis-inclusive-range-patterns can easily be made to Just Work once we have pattern parentheses. It would be a shame to not autofix what we definitely can, just because other lints in the same lint-group are harder to solve.

@Manishearth
Copy link
Member

We can recommend --idioms to be something you run multiple times? While cargo fix for migration should report unfixed warnings, --idioms doesn't have to -- so we can have this transition by gradually marking more suggestions as applicable, and in the end just recommend a single pass (but folks following along can run it multiple times)

@alexcrichton
Copy link
Member Author

@zackmdavis that's a good point yeah, if the experience is "pretty good" I think we can definitely turn it on by default. This may just be a case where we're more selective about lints in the rust_2018_idioms lint group!

If we trim down the lint group I think it'd also be a good solution to this problem, and then we could add more lints once they've been vetted and are known to have good suggestions.

@zackmdavis
Copy link
Member

This may just be a case where we're more selective about lints in the rust_2018_idioms lint group!

It also seems unfortunate to conflate "unidiomatic code that we can lint for" with "unidiomatic code that we can lint for and have an autofixable suggestion for". There may just be cases where the fix requires human judgment. (I fear unreachable-pub and macro-use-extern-crate will end up in this category unless someone comes up with a very clever suggestion implementation.)

Should --idioms even be a separate flag? In my dream world, cargo fix --prepare-for 2018 fixes compatibility lints and fixes only the idiom lints that it feels certain about. Then people who really want to be idiomatic also have #![warn(rust_2018_idioms)] in their code, so they can manually fix the remaining idiom lints.

@nikomatsakis
Copy link
Contributor

Should --idioms even be a separate flag? In my dream world, cargo fix --prepare-for 2018 fixes compatibility lints and fixes only the idiom lints that it feels certain about. Then people who really want to be idiomatic also have #![warn(rust_2018_idioms)] in their code, so they can manually fix the remaining idiom lints.

That sounds appealing to me

@withoutboats
Copy link
Contributor

I agree that --idioms as a separate flag seems like an overcomplicated API. For the purpose of cargo fix, 2018 lints I think should fall into two categories:

  • cargo fix does a good job of fixing them. They're turned on when you run cargo fix --prepare-for 2018
  • cargo fix stumbles too often trying to fix them, or can't fix them at all. They're not turned on when you run cargo fix --prepare-for 2018.

Having made that division, we need to ask if this is a problem for 2018 if the set of lints in the first group is not big enough, if there are critical lints that we currently don't automatically fix.

@Manishearth
Copy link
Member

That doesn't quite work. There already is an existing distinction of lints:

  • Lints which must be applied on 2015 that make it possible to migrate to 2018. They do not stop your code compiling on 2015. These are migration lints.
  • Lints which must be applied on 2018. These may stop your code compiling on 2015. These are idiom lints.

Some lints, like dyn trait, can belong in either group; but for many there's only one logical category.

This distinction must be made because the idiom lints cannot be applied on 2015, and the migration lints cannot be applied on 2018 (if you're triggering migration lints your code will not compile on 2018).

This can't all be handled by cargo fix because there's the step of bumping the crate edition in between. And that can only be done once all the edition migration fixes have been applied, which won't always be the case -- there's a bit of manual work here for some crates especially whenever macros are involved.

What we can do is make cargo fix try to do this, but it's going to fail at times and we'll need a manual workflow.

It's not overcomplicated; some complication here is necessary. We can make the common case smooth but we definitely need this distinction.


TBH I'm getting a bit frustrated here, because this is the third time we're having this discussion. When we started this whole edition lint work I pointed out we would need the migration/idiom category split, folks disagreed, and eventually independently came to the conclusion that yes, we do need these two categories (or something roughly similar). That we're back on that discussion, starting from scratch, is frustrating.

@alexcrichton
Copy link
Member Author

FWIW there is no --idioms flag today in cargo fix. It's definitely a possible extension, but just want to be sure we understand that there's not a concrete thing in existence to talk about for the time being!

As @Manishearth mentioned we won't be able to lump everything in with the initial --prepare-for step. Additionally we're having enough trouble with the lints required to run there, I'd rather not add more "nice to haves" until we're even more confident than we are today.

That all means that we're required to have two "fixing steps" (before and after the edition switch). Orchestrating this all automatically is the subject of rust-lang/cargo#5778 to hopefully make it less confusing.

I'm personally a fan of adding a --idioms-for 2018 flag to cargo fix. That would enable the rust-2018-idioms group for you and avoid needing to modify code with #![warn] business that probably shouldn't stay there. That, plus being selective about what's in the lint group (e.g. macro-use would not be in there, but dyn Foo would be in there) I think means we could continue to mention this even at the time of the 2018 edition release. We could even mention that we don't handle all idioms, but more will be coming as time passes!

So to summarize:

  • Add --idioms-for 2018 flag to cargo fix
  • Remove overeager lints from the rust_2018_idioms group (like the macro use lint)

and... that's it! Otherwise we'd continue to mention the idioms lint, just as cargo fix --idioms-for 2018 instead of #![warn(rust_2018_idioms)]

@alexcrichton
Copy link
Member Author

I've opened rust-lang/cargo#5843 for Cargo to add a --idioms flag, #52926 to trim down the idioms lint group to "good lints to hvae on by default", and rust-lang/edition-guide#70 to update the edition transition documentation.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Aug 1, 2018
These migration lints aren't all up to par in terms of a good migration
experience. Some, like `unreachable_pub`, hit bugs like rust-lang#52665 and unprepared
macros to be handled enough of the time. Others like linting against
`#[macro_use]` are swimming upstream in an ecosystem that's not quite ready (and
slightly buggy pending a few current PRs).

The general idea is that we will continue to recommend the `rust_2018_idioms`
lint group as part of the transition guide (as an optional step) but we'll be
much more selective about which lints make it into this group. Only those with a
strong track record of not causing too much churn will make the cut.

cc rust-lang#52679
pietroalbini added a commit to pietroalbini/rust that referenced this issue Aug 1, 2018
…li-obk

rustc: Trim down the `rust_2018_idioms` lint group

These migration lints aren't all up to par in terms of a good migration
experience. Some, like `unreachable_pub`, hit bugs like rust-lang#52665 and unprepared
macros to be handled enough of the time. Others like linting against
`#[macro_use]` are swimming upstream in an ecosystem that's not quite ready (and
slightly buggy pending a few current PRs).

The general idea is that we will continue to recommend the `rust_2018_idioms`
lint group as part of the transition guide (as an optional step) but we'll be
much more selective about which lints make it into this group. Only those with a
strong track record of not causing too much churn will make the cut.

cc rust-lang#52679
bors added a commit to rust-lang/cargo that referenced this issue Aug 2, 2018
Add a `--edition-idioms` flag to `cargo fix`

This, like `--prepare-for`, will be part of the transition guide which
automatically applies the necessary lint group from the compiler to associated
code.

cc rust-lang/rust#52679
@alexcrichton
Copy link
Member Author

I believe this is now done and good to go!

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
Projects
None yet
Development

No branches or pull requests

7 participants