Repository navigation
Should the rust_2018_idioms lint group be mentioned in documentation? #52679
Description
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!
Activity
aturon commentedon Jul 24, 2018
cc @nikomatsakis @Mark-Simulacrum @Centril @Manishearth
Manishearth commentedon Jul 24, 2018
IIRC one of the original plans was something like:
cargo fix --idioms. This turns on these lints and fixes as many as possible. It won't complain about unfixable things (for now).Centril commentedon Jul 25, 2018
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 commentedon Jul 25, 2018
I think it's worth distinguishing what behavior we want from something like
cargo fix --idiomsfrom 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 commentedon Jul 25, 2018
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 commentedon Jul 26, 2018
@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_idiomslint 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 commentedon Jul 26, 2018
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-pubandmacro-use-extern-cratewill end up in this category unless someone comes up with a very clever suggestion implementation.)Should
--idiomseven be a separate flag? In my dream world,cargo fix --prepare-for 2018fixes 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 commentedon Jul 26, 2018
That sounds appealing to me
withoutboats commentedon Jul 26, 2018
I agree that
--idiomsas a separate flag seems like an overcomplicated API. For the purpose of cargo fix, 2018 lints I think should fall into two categories:cargo fixdoes a good job of fixing them. They're turned on when you runcargo fix --prepare-for 2018cargo fixstumbles too often trying to fix them, or can't fix them at all. They're not turned on when you runcargo 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 commentedon Jul 26, 2018
That doesn't quite work. There already is an existing distinction of 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 fixtry 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 commentedon Jul 26, 2018
FWIW there is no
--idiomsflag today incargo 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-forstep. 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 2018flag tocargo fix. That would enable therust-2018-idiomsgroup 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, butdyn Foowould 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:
--idioms-for 2018flag tocargo fixrust_2018_idiomsgroup (like the macro use lint)and... that's it! Otherwise we'd continue to mention the idioms lint, just as
cargo fix --idioms-for 2018instead of#![warn(rust_2018_idioms)]alexcrichton commentedon Jul 31, 2018
I've opened rust-lang/cargo#5843 for Cargo to add a
--idiomsflag, #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.rustc: Trim down the `rust_2018_idioms` lint group
Rollup merge of rust-lang#52926 - alexcrichton:trim-idioms-lints, r=o…
Auto merge of #5843 - alexcrichton:idioms-for, r=alexcrichton
alexcrichton commentedon Aug 3, 2018
I believe this is now done and good to go!