Epoch lints don't handle macros very well

[Manishearth]

If a lint originates in a macro, epoch lints will still emit a suggestion.

This suggestion often works, especially when the linted code comes from within the macro, entirely.

Given that these are backcompat lints we have to emit them even if the current crate cannot fix them.

I suggest stabilizing the approximate flag on suggestions, so that rustfix can differentiate between these and provide a mode where it fixes everything but prompts for these. Either that, or we don't emit suggestions at all in these cases.

https://github.com/killercup/rustfix/issues/57 tracks a bug in rustfix that occurs for macros where duplicate suggestions apply to the same place.

cc @nikomatsakis

[est31]

In the epochs RFC discussion, we got promised that macros will get "epoch hygiene" aka using a macro from an older epoch that emits old epoch stuff inside a crate from the newer epoch (and vice versa) will work. Can't this hygiene be extended to lints?

[Manishearth]

idk. I don't think epoch hygiene can be perfect given that the error can be caused by the interactions of macros from different crates. Epoch breakages can be conservative, but lints should go the other way and try to warn more even for stuff you can't directly fix.

The problem is not that we want to precisely detect these things, it's that we want to emit good suggestions.

Also, we don't have hygiene. yet, but we want the epoch lints to exist early.

[petrochenkov]

The whole "deprecation" part of the epoch RFC: "Let's invent problems so we have something to solve."

[nikomatsakis]

I think that hygiene questions are orthogonal here anyhow. The macro may well originate in the same crate, for example.

[nikomatsakis]

I also think this concern is orthogonal from epochs, basically -- this seems to apply equally well to any lint. Epochs are only different in that we want to try harder; but having lints that just 'turn off' around code in macros is pretty suboptimal (even if sometimes the most expedient thing).

[nikomatsakis]

I suggest stabilizing the approximate flag on suggestions, so that rustfix can differentiate between these and provide a mode where it fixes everything but prompts for these. Either that, or we don't emit suggestions at all in these cases.

This sounds reasonable, but what is this flag? I guess it's something you added in #47540... feels like we need a tracking issue of some kind for this feature? Then we can do an FCP? And/or an RFC?

(I still feel grumpy that we have so little process around our JSON output, which then makes it hard to specify small incremental changes to it.)

This feels like @rust-lang/dev-tools concern, in any case? Or possibly @rust-lang/compiler -- it's right on that intersecting line.

[Manishearth]

Yeah the json field is behind a -Z flag so we could add it backwards compatibly. It can be RFCd, and we can move towards using it for everything in rustc. Default to not-approximate and use it whenever.

Mini-rfc (perhaps just an FCP on a PR?) should be ok imo.

---

Yeah, the concern is orthogonal from epochs; like, it's something we have tried (and failed) to solve in clippy as well because rustc just doesn't (or didn't, at the time) have enough expansion info to solve this correctly. Clippy currently just bails on macro-y things.

The concern is a major one for epochs because we can't brush it away with "it's ok to have false negatives on a lint"; epoch lints and compat lints have to catch everything.

[est31]

Note that I'm refferring to @nrc's comment in the epoch RFC discussion. The concern is very much not orthogonal to epochs. I think it is even more important for epochs than the raw identifiers RFC is.

[nikomatsakis]

@est31 do you mean that "epoch hygiene" is not orthogonal to epochs, or the need to be able to identify "approximate" suggestions?

[Manishearth]

One thing we noticed yesterday is that at least for this epoch the only breaking change is new keywords, which will work in the lexer, so epoch hygeine is not a problem (I assume macros are exported as tokens and not source).

However, if we decide to enable dyn trait before the epoch, we have a small hygeine concern about the dyn ::foo construction -- if people on older epochs had modules named dyn, and macros that constructed them across crates, we could have a problem. This one is pretty theoretical, but some of the other similar features that we stabilize pre-epoch in a slightly nerfed form may have more practical breakages.

[est31]

@nikomatsakis

do you mean that "epoch hygiene" is not orthogonal to epochs

yes, I meant that. Think of a future date where you have multiple epochs each with its own style. Here, writing code in a macro that works on all epochs is hard, you need hygiene. Avoiding to use the keywords from those epochs as identifiers should be quite easy on the other hand though. That's why I believe epoch hygiene is so important.

[nikomatsakis]

@Manishearth

One thing we noticed yesterday is that at least for this epoch the only breaking change is new keywords, which will work in the lexer, so epoch hygeine is not a problem (I assume macros are exported as tokens and not source).

I'm a bit confused by this. I mean if you have some macro that generates a local variable named catch, for example, you're saying that won't cause an issue?

However, if we decide to enable dyn trait before the epoch, we have a small hygeine concern about the dyn ::foo construction

Yeah so right now you have to do dyn (::foo) -- which we should incorporate into our suggestion, btw! This is kind of annoying and we could certainly switch the interpretation of those tokens post-epoch (or just only enable dyn form in the new epoch). I definitely like giving people the opportunity to migrate to the new syntax (possibly with extra parens) before opting in to the new epoch.

Clearly we can't give warnings unless there is a new form available for you to migrate to! So either we should limit dyn (and its assocaited warnings) to the new epoch, or else change how we handle the corner case.

(I do suspect the breakage here is largely theoretical, but then isn't the point of the epoch to enable us to avoid even theoretical breakage?)

cc @rust-lang/core -- the dyn Trait makes for an interesting "case study" of how to handle an epoch transition, it'd be good to make sure we are all on the same page.

[Manishearth]

I'm a bit confused by this. I mean if you have some macro that generates a local variable named catch, for example, you're saying that won't cause an issue?

Yeah, because we store the macro as tokentrees (MacroRulesMacroExpander), macros written in old crates will have catch be an Ident, and used as an ident in new crates. IIRC rust doesn't check for keywordness during parsing , aside from contextual keywords, which don't matter here (you can already use union as a variable name)

If macros worked in all positions we literally wouldn't need the identifier RFC, we could just export macros called catch_ident!(), etc

Yeah so right now you have to do dyn (::foo) -- which we should incorporate into our suggestion,

We do? 0cb3672

warning: trait objects without an explicit `dyn` are deprecated
 --> test.rs:4:12
  |
4 | fn foo(x: &::Foo) {
  |            ^^^^^ help: use `dyn`: `dyn (::Foo)`

This is kind of annoying and we could certainly switch the interpretation of those tokens post-epoch

That's going to happen anyway, the moment dyn becomes a keyword, dyn::foo can only be a trait object. We can, of course, tweak the suggestion so that it doesn't emit parens in epoch 2018.