Macro invocations bypass new keyword identifier checks

[alexcrichton]

This code currently compiles on nightly:

#![warn(rust_2018_compatibility)]
#![feature(rust_2018_preview)]

macro_rules! r#async {
    () => ()
}

fn main() {
    async!();
}

.. with no warnings, but it should get a warning about the macro invocation!

cc @zackmdavis, do you know if there's a good location we can run lints before macro expansion? I think the bug here is that the macro is fully expanded by the time we hit the lint passes, so it's not even present in the AST.

[Centril]

So there's an unresolved question in #50412 about try!(..).
Depending on what we want, this may actually be the intended behavior for try!(..) at least.
For consistency, it may also be what we want for async.

cc @scottmcm, @nikomatsakis, and @est31.

[Centril]

Also cc @rust-lang/lang.

[Mark-Simulacrum]

Per #52602 (comment) I've nominated this and marked as p-high to ensure we can get some eyes on this; I've also added T-compiler to hopefully assign someone who can do the legwork once lang arrives at a decision (I believe it's not quite clear what we want here).

[eddyb]

@Centril I disagree in the general case, because Leyword Expr is not banned in the grammar, and Expr can start with !, so this is a significant distinction between keywords and other identifiers.

IMO you should have to use r#async! to invoke that macro, if we even allow it at all.
cc @petrochenkov

[Centril]

@eddyb hmm; for async you have to write async { .. }; i.e, there is no async <expr> expression form so you can't write async ! ( foo() ) or something like that.

I think we should try to be consistent in what we do, so if we say ixnay on async!(..) then we should say ixnay on try!(..) and if we allow one we should allow both. As for what we should do, I don't have any strong feelings either way.

However, a point for consideration is that throw!(..) exists in the failure crate. If we hypothetically would like throw $expr as an expression construct and throw as a keyword in the future, then we should consider whether we would allow throw!(..), and what bearing this would have on try!(..) and async!(..).

[eddyb]

@Centril Won't try! work only in Rust2015 and require r#try! in Rust2018?
Anything else seems quite inconsistent and complicating the grammar further.

[Centril]

@eddyb I mean, that's what the choice before us is. We can make try!(..) work in Rust 2018 if we want technically, can't we?

• I agree that not allowing try!(..) would be consistent thing to do.
• On the other hand, we might want to reduce breakage.
• However, another point is that it might be beneficial to break try!(expr) as further encouragement in favor of expr? and rustfix people towards the latter.

There's arguments in favor of and against each choice here. 🤔

[petrochenkov]

@eddyb

• catch { ... } blocks could work on Rust 2015 with catch being a context-sensitive identifier and zero practical breakage.
• However, catch was used as a showcase for raw identifiers and that eventually lead to enormous waste of human time which is RFC: Reserve try for try { .. } block expressions rfcs#2388 and all related discussions.
• As a result catch { ... } is now renamed to try { ... }, it still can work on Rust 2015 with try being a context-sensitive identifier and zero practical breakage, and try!(...) working on both editions.

[eddyb]

If you're switching to the Rust 2018 edition, I'd argue try! should not resolve (but we don't have the machinery to make it work like that). Definitely want to rustfix all code to ?.
That would allow us to reserve the keyword and avoid adding more grammar corner cases.

[pnkfelix]

Sounds like there is a decision that needs to be made here, given the options laid out by @Centril

Tagging with T-lang and adding I-needsdecision.

(Also, once a decision is made, we also need to assign the ticket to someone to actually implement that decision...)

[nikomatsakis]

I am fine with having try! work as a special case, but also fine with not doing so.

The more important problem seems to be building up the machinery to make this transition happen "pre-macro-expansion".

Seems doable, though, but it requires some special-case handling presumably.

[nikomatsakis]

I think we should:

• In Rust 2015, add a migration lint that converts try to r#try
  • Suggesting ? may not always succeed because of type inference interactions
• In Rust 2018, treat e.g. try as a keyword and hence give a hard error
• In Rust 2018, add an idiom lint that complains about usage of the r#try! macro
  • Suggests a rewrite to use ? — but this will sometimes fail because of type inference interactions

[nikomatsakis]

@rfcbot fcp merge

The problem here is that we permit some keywords (but not all) to be used as macros. Specifically, (I believe) we permit only the new 2018 keywords (e.g., try, await) to be used as a macros.

To correct this inconsistency, I proposed that we do the following:

• In Rust 2015, add a migration lint that converts try to r#try (and so forth)
  • Suggesting ? may not always succeed because of type inference interactions
• In Rust 2018, treat e.g. try as a keyword and hence give a hard error
• In Rust 2018, add an idiom lint that complains about usage of the r#try! macro
  • Suggests a rewrite to use ? — but this will sometimes fail because of type inference interactions

[rfcbot]

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

• @Centril
• @Zoxc
• @aturon
• @cramertj
• @eddyb
• @estebank
• @joshtriplett
• @michaelwoerister
• @nagisa
• @nikomatsakis
• @nrc
• @oli-obk
• @petrochenkov
• @pnkfelix
• @scottmcm
• @varkor
• @withoutboats

Concerns:

• try is still used resolved by Macro invocations bypass new keyword identifier checks #53686 (comment)

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[withoutboats]

I wanted to point out that what Niko proposes is exactly how editions were designed to work: if an API is using a new keyword, it gets swapped out for the r# form in the new edition. The only quirk here is that try! ideally could be replaced by ? already, except for type inference.

[petrochenkov]

@rfcbot concern try is still used

try was the recommended way to do error handling, so it's still there in code that's somewhat old and is not actively maintained. That code stays on 2015, so it will be okay it the migration lint is not enabled by default.
A number of people explicitly stated the desire to continue using try! over ? because it's more visible / searchable / better fit for their code priorities. In this case the suggestion to use r#try! if they want to migrate to 2018 looks like mockery to me, basically.

[zackmdavis]

Furthermore, for those who do want to migrate, they can easily do so with cargo fix; I don't see a since instance in which cargo fix would fail to convert try to r#try

It's worth noting that Python also had a builtin zero-configuration migration tool that had no trouble with print statements. I'm not actually sure what the moral here is. (Some users will complain about any migration work, no matter how trivial?)

[nikomatsakis]

@petrochenkov

We discussed your comment in the @rust-lang/lang meeting today. As you pointed out, there are basically two reasons that people might want to use try!:

• Older code that is not maintained.
• Newer code, where people just don't like ?.

Older code is not an issue: the migration lints are not on by default, and anyway, they are lints. Such code continues to work as it ever did. In newer code, people could of course opt to continue using Rust 2015, though I would hope they don't. Otherwise, they can use ?, which would be the idiomatic thing to do, or create their own macro.

This trade-off seems perfectly acceptable to me! One purpose of the edition, after all, is to allow us to "consolidate" idiom shifts -- such as the shift from try! to ? -- that have occurred. This in turn frees up the syntax (in this case, the try keyword) to be put to new uses.

Basically, in moving to merge, our judgement was that uses of try! were already distinctly unidiomatic, and hence it made sense to move forward with the "cleaner" Rust 2018 design, in whichtry simply acts as a keyword like any other.

In any case, we are operating under definite time pressure here. Normally we would wait for the formal objection to be cleared, but since we're trying to get this settled for the upcoming Release Candidate, and believe the objection has a reasonable response, we are going to go ahead and make a PR. But please, if you feel the objection is not sufficiently dealt with, speak up!

[petrochenkov]

@rfcbot resolve try is still used

[Mark-Simulacrum]

Can someone assign themselves to this issue? I'm not sure exactly what's involved in making the necessary changes here, so it's hard to estimate the difficulty/scope of this task.

[zackmdavis]

do you know if there's a good location we can run lints before macro expansion?

Looks like this was implemented recently by @mark-i-m (libsyntax/early_buffered_lints.rs)?

[mark-i-m]

Yep, also see https://rust-lang-nursery.github.io/rustc-guide/diag.html#linting-even-earlier-in-the-compiler

[nikomatsakis]

Is anyone working on this issue? I had thought to start on it today but didn't get to it

[nikomatsakis]

My understanding is that, today, macro invocations are actually still present in the AST. I wonder if we might do this in a different way-- basically by walking the AST post expansion and looking for cases that will become a problem?

[nikomatsakis]

Ah, this is interesting: this might be a case where the "cross-crate edition hygiene" becomes relevant in practice. In particular, if I use a macro from another crate that uses try!.

[nikomatsakis]

Well, "wait and see" still applies, I suppose

[nikomatsakis]

My understanding is that, today, macro invocations are actually still present in the AST.

If that is true, I can't figure out where from some brief reading of the code :)

[nikomatsakis]

Hmm, I see that the existing keyword lint is registered as a "pre-expansion" lint -- but perhaps some expansion happens before? It's certainly true that this example gets no warnings today.

[nikomatsakis]

OK, I think I found the problem. Once I verify I am correct, I'm going to open a minimal PR first and file a bug for a more proper fix. (I think the bug is more in the visitor infrastructure than anything else.)

[nikomatsakis]

OK, spoke too soon, but I do actually have it fixed now. 😛 Now trying to figure out if the first fix that I did is truly needed or not...