Rust: 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 https://github.com/rust-lang/rust/issues/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 https://github.com/rust-lang/rust/pull/52602#issuecomment-415889173 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 https://github.com/rust-lang/rfcs/pull/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:

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

Concerns:

• try is still used resolved by https://github.com/rust-lang/rust/issues/53686#issuecomment-419251486

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.

[petrochenkov]

My suggestion for try is same as it was for catch - context sensitive identifier with special meaning in block expressions.

Or a context sensitive identifier with special meaning everywhere except for macro calls if you really want to reserve it harder for later use in other syntactic constructions beside try blocks.

[Centril]

@petrochenkov

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.

For me it's not the right priority to complicate the grammar of the language and introduce a whole new class of "almost real keyword" to satisfy that desire.

In particular, allowing try!(expr) to mean the invocation of try! instead of try expr.not() means that we take the choice away from us to allow people to omit the braces in the future (not saying that we should necessarily do this; but I want to retain the possibility to do so).

I think that for new code, the people who want a more visible error handling mechanism can introduce a new macro with a different name; such as:

macro_rules! check { ($e: expr) => { $e? } }

Other potential names are:

• test
• attempt

In this case the suggestion to use r#try! if they want to migrate to 2018 looks like mockery to me, basically.

It is a bullet-proof approach that makes their code compile in the 2018 edition; they can then introduce the macro above and find/replace all invocations of r#try! to check! in one go. That doesn't seem to me a heavy burden.

[joshtriplett]

I don't think it's unreasonable to parse try { as a try block, and try! as the macro, for the sake of compatibility.

I've had multiple people mention that one of the biggest pain points of the Python 2 to Python 3 transition was the change to the "print" function; it's not that people didn't like the new version, it's that it was a lot of work to change code over just to keep working code working. If we can make the old code keep working, let's.

[Centril]

@joshtriplett The python 2 / 3 situation doesn't apply to us because folks can combine crates with different editions. 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. So the work needed to transition with respect to try! is just invoking cargo fix.

What I realized after last week's meeting was that if we support try!(expr) in the 2018 edition, then not only will it complicate the grammar and introduce inconsistencies, but it will also take the design choice away from us to let folks write:

let foo = try a.checked_add(b)?.checked_add(c)?; // Notice the lack of braces.

We wouldn't be able to do this because if a user wrote try!(expr), then it would be interpreted as a macro invocation and not:

try // keyword
! // not operator;  expr : "!" expr ;
( // expr : "(" expr ") ;
expr
)

Therefore, I am increasingly skeptical that try!(..) should work in 2018.

[scottmcm]

This plan sounds good to me. I like pushing towards ? now, since we can always re-enable try! if we need to, but I'd rather not set ourselves up from the start to hit exactly the same discussion next edition. The keyword policy decision was to use real keywords for edition-gated features; let's stick with that.

[withoutboats]

I think that for new code, the people who want a more visible error handling mechanism can introduce a new macro with a different name

I agree with this.std::try! is a deprecated API. If enough users want an API with the same functionality, they should create one on crates.io. But we've decided to reserve the try as a keyword in 2018, so if they want that crate to work well with 2018 clients, I'd recommend using a different name.

[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...