Cargo fix --prepare-for should only apply edition fixes

[alexcrichton]

Originally reported as rust-lang/rustfix#129

---

As per rust-lang/rustfix#120 it applies all suggestions, which include the builtin lints.

[alexcrichton]

@Mark-Simulacrum on IRC suggested we pass -A warnings -W rust-2018-compatibility which I think will work like a charm here! (we should test though)

[killercup]

@alexcrichton I'll give it a try, but this previously didn't work

[alexcrichton]

Oh bah you are indeed correct (still!).

That's due to this block of code.

That means probably we could either:

• Fix this as a bug in rustc (maybe having lots of unintended fallout)
• Pass a whole slew of -A and hope we get most of the ones that matter
• Simply don't auto-fix anything that doesn't pass our whitelist of what to look at

I'm leaning towards the last option now, but it'd probably be easier to go with the second. @killercup do you have a preference?

[killercup]

Great research!

I think the last option might be one with the least issues going forward – adjusting an allow-list when people add new edition specific lints is trivial. What do you mean with "easier to go with the second" – tracking (and keeping track of!) all enabled-by-default lints in rustc for the foreseeable future? Sheesh!

[alexcrichton]

Oh more just easier to implement because it's just modifying a string that's already in the code :)

I'm cool w/ the third option though!

[alexcrichton]

Bumping this to the RC1 milestone

[dwijnand]

I had a go at fixing this in #5948 (using the third option), but Alex notes:

unfortunately the list of lints in rust-*-compatibility is changing over time pretty rapidly, so while it's possible in a pinch we could go with that strategy I'd personally prefer to avoid doing so

but suggests we could still go with this option by changing the compiler to get the lint to expose its lint group and using that.

But it might be fun to see how to fix rustc so -A warnings -W rust-2018-compatibility honours precedence, and see what kind of fallout that has.

Either way I'd need some rustc mentorship.

[alexcrichton]

Yeah and to recap what we discussed, two ideas I have for fixing this in rustc are:

• First, fix how rustc handles -A warnings -W rust-2018-compatibility to actually work
• Second, include the lint group in the diagnostic message somehow so we can read it out

I'm not sure how to do the latter but the former is related to this block of code. I think to fix that we'd basically want to remove that block entirely and handle -A warnings elsewhere (e.g. setting all lints to "allow" that are otherwise "warn" early on).

@dwijnand are you interested in doing the -A warnings strategy? If so I can try to help out to guide you in rustc for that!

[dwijnand]

Yep!

[alexcrichton]

Ok so the general idea is that during linting we walk the entire AST, keeping a set of the current lint levels for all lints. This set is modified whenever we encounter lint attributes, and is otherwise automatically managed whenever we walk up/down the AST.

Overall this means that when issuing a lint you don't actually say anything about the lint level. Only during the linting passes do we later realize what the lint level actually is and that dictates how the lint itself is issued. Currently there's a hardwired statement (linked above) which simply treats all warnings as being overriden by the lint warnings lint level.

I think instead we'll want to tweak how the "warnings" lint is treated. Instead of having it override, we should have processing of the "warnings" lint be special (whenever we see an attribute or a CLI flag changing it). Changing the warnings lint should change all lints that are currently set to warn to allow. This means that we'd process allow(warnings) in a scoped fashion rather than a "let's force change things" fashion.

Does that makes sense?

If so, I think this will change by tewaking this enum to have special dedicated fields in each variant for "the warnings lint changed". The special warnings handling block would be removed and instead the get_lint_id_level function would take -A warnings into account.

[dwijnand]

After several passes, yes.

But to drive it home could you give me a few working and not working examples (expected vs intended behaviour)? I'm actually not too familiar with rust's lints or rustc's CLI. Thanks!

[alexcrichton]

Sure!

• This should generate a warning
• This should continue to not generate a warning
• This should generate a warning

And... that's probably mostly it!

The handling of levels in crate is pretty similar to handling of levels on the CLI, so these can be translated roughly to cli flags as well

[dwijnand]

Alright, thank you. I'll see what I can do.

[dwijnand]

Hey @alexcrichton,

So after some initial troubles getting started with rustc development, I've finally been able to play with this part of the compiler to see if I can figure it out.

Unfortunately progress is quite slow. Could you explain to me more what you had in mind when you said the get_lint_id_level function would take -A warnings into account?

[alexcrichton]

Sure yeah! So get_lint_id_level is tasked with the job of given a node id a crate (like an expression or something like that), determining what the lint level is of a particular lint. To do this it starts at the node and walks upwards. (the current node is represented by the aux map).

The lint level is then determined by the first node we actually find. As we walk upwards we'll find #[allow] (lint attributes) and finally the last thing we walk up to is the command line itself. If all else fails we then return LintSource::Default, which indicates that we should use whatever the default level is.

Instead what needs to happen is what while we're doing this traversal we need to also keep track of the state of the Warnings lint (as configured by the innermost option). If the warnings lint is set to allow and we othrewise figure out that this specific lint is set to warn then it should actually be forced to allow. If the lint in question becomes deny though we just return deny.

Does that make sense?

[alexcrichton]

@dwijnand circling back around to this, it looks like #6178 is another instance of this. I'm guessing though that not much progress was made on the rustc side of things?

[dwijnand]

No rustc continues to defeat me, for now. But being a sucker for the sunk cost fallacy I might come back to this before the year is out...

[alexcrichton]

Ok no worries! Feel free to reach out with any issues of course