Remove NtExpr.

[nnethercote]

By using invisible delimiters.

r? @ghost

[nnethercote]

I was playing around with this just to see what happened. #96627 showed that avoiding Interpolated for NtIdent compiled ok but caused lots of error message to degrade. Here I've done much the same for NtTy and the results are intriguing. There were eight ui test failures to start with.

    [ui] src/test/ui/did_you_mean/bad-assoc-expr.rs
    [ui] src/test/ui/did_you_mean/bad-assoc-pat.rs
    [ui] src/test/ui/did_you_mean/bad-assoc-ty.rs
    [ui] src/test/ui/parser/macro/issue-37113.rs

For these four, after the change there is one less error than before. And I think the new behaviour is better. I can't see why the relevant lines were errors. Perhaps the parser has one or more places where an Interpolated should have been handled.

    [ui] src/test/ui/macros/stringify.rs
    [ui] src/test/ui/proc-macro/capture-unglued-token.rs

For these ones a lot of the stringified outputs have changed. A lot are worse, with worse spacing. But a few are arguably better. There's even a FIXME that this change seemingly fixes.

    [ui] src/test/ui/parser/macro/trait-object-macro-matcher.rs
    [ui] src/test/ui/tuple/tuple-struct-fields/test2.rs

These are ones where the error messages have changed a little for the worse.

After the ui tests are made to pass, other tests work fine up to the debuginfo tests. After that the rand-0.8.5 crate is compiled, and there's a new compiler error. Here's a reduced test case showing the problem:

// Current rustc and new version both reject this like so:
//
// error: `<` is interpreted as a start of generic arguments for `u32`, not a shift
//   --> f.rs:15:14
//    |
// 15 |     1 as u32 << 24
//    |              ^^ -- interpreted as generic arguments
//    |              |
//    |              not interpreted as shift
//    |
// help: try shifting the cast value
//    |
// 15 |     (1 as u32) << 24
//    |     +        +

fn sample2() -> u32 {
    1 as u32 << 24
}

// Current rustc accepts this, new version doesn't.
macro_rules! f {
    ($u_scalar:ty) => {
        fn sample1() -> u32 {
            1 as $u_scalar << 24
        }
    }
}

f! { u32 }

fn main() {
    let _ = sample1() + sample2();
}

Again, I think the new behaviour is reasonable.

This doesn't seem possible to land, but it does suggest that the current use of Interpolated might be resulting in some bugs.

cc @petrochenkov

[petrochenkov]

The 1 as $u_scalar << precision breakage may also be fixed by not dropping Invisible delimiters.

If we assume that $u_scalar is e.g. u64 (I didn't check), then u64 would be an identifier, so we'd continue parsing < as a start of generic arguments after it (*), but «u64» wouldn't be an identifier, so the parsing shouldn't proceed to < in the same way.

(*) The grammar is IDENT < GENERIC_ARGS >.

[petrochenkov]

Conclusion: most of the changes in tests will disappear if Invisible delimiters are not dropped by the parser.

[petrochenkov]

For quick testing you can replace the invisibles with regular parentheses () in this case because (TYPE) is also a type and the effect should be very close.

[nnethercote]

I have updated. As well as not producing Interpolated tokens for NtTy, the parser now also does not drop invisible delimiters, and there is more handling of invisible delimiters. These parser changes seem to mostly work, though I'm not confident I have all the details exactly right.

On one hand, it's encouraging that I've managed to get it down to only four ui test failures. On the other hand, there are many non-terminal kinds remaining. And this doesn't seem like something that can be done incrementally... all the extra handling of invisible delimiters needs to be in place once we stop dropping them. It seems hard to make such a big change all at once without causing lots of subtle breakage .

[petrochenkov]

@nnethercote
What prevents using the strategy of preserving Invisibles conditionally, like

fn bump(&self, preserve_invisible: bool) { ... }

?

I'm not ready to take a responsibility for reviewing a change that does all of this in one go.
I think we should convert all macro_rules matchers in separate steps, starting from ty or expr or pat where invisibles are equivalent to parentheses, then wait for regressions or run crater, then continue with other matchers.

Every such step may potentially require a language team involvement.
Every such step may also cause breakage requiring additional flags in Delimiter::Invisible to mitigate (due to things like self.look_ahead(1, |t| matches!(t.kind, NtTy(..)) or matches!(self.prev_token.kind, NtTy(..))).
It's going to be a process taking some time.

[nnethercote]

@nnethercote What prevents using the strategy of preserving Invisibles conditionally, like

fn bump(&self, preserve_invisible: bool) { ... }

?

I understand the motivation here. But we always get one token in advance. When we're at a place where we want to allow an invisible delimiter, e.g. in parse_bottom_expr, we've already ignored it in the previous bump call. At that point we really want to say "reget the last token, allowing invisible delimiters this time".

[petrochenkov]

I need to think how to better stage the implementation here.
Maybe the invisible delimiters can be conditionally removed at some earlier stage, for example in fn parse_ast_fragment immediately after leaving the proc macro.

[petrochenkov]

So, to split the implementation into multiple part we need bump to preserve Delimiter::Invisibles only if they come from macro_rules ty (NtTy), and skip all other invisible delimiters.

To do that we should be able to simply keep an extra flag in Delimiter::Invisible indicating that the delimiter must not be skipped.
proc_macro::Delimiter::None doesn't need such a flag, because this flag will be preserved inside rustc, but will be "laundered" if passed through a proc macro, exactly how NtTys disappear when passed through proc macros.

Once all nonterminal tokens are migrated we'll be able to switch this flag to "never skip" and process invisible delimiters from proc macros correctly too.

[nnethercote]

Good suggestion. I have added the flag to Delimiter::Invisible, and switched from removing NtTy to removing NtExpr. I have uploaded the code in this PR. There are currently 23 test failures, in three main categories:

• Ones where the error message is slightly worse because of minor span differences.
• Proc macro ones where I haven't yet updated the PRINT-BANG output, which will be easily fixed.
• More serious cases where things don't compile that should.

[nnethercote]

The current test failures:

    [ui] src/test/ui/borrowck/issue-25793.rs
    [ui] src/test/ui/conditional-compilation/cfg-attr-syntax-validation.rs
    [ui] src/test/ui/inference/deref-suggestion.rs
    [ui] src/test/ui/issues/issue-26094.rs
    [ui] src/test/ui/issues/issue-26237.rs
    [ui] src/test/ui/macros/issue-29084.rs
    [ui] src/test/ui/mismatched_types/issue-26480.rs
    [ui] src/test/ui/parser/extern-abi-from-mac-literal-frag.rs
    [ui] src/test/ui/parser/float-field-interpolated.rs
    [ui] src/test/ui/parser/macro/trait-non-item-macros.rs
    [ui] src/test/ui/proc-macro/capture-macro-rules-invoke.rs
    [ui] src/test/ui/proc-macro/expr-stmt-nonterminal-tokens.rs
    [ui] src/test/ui/proc-macro/issue-75734-pp-paren.rs
    [ui] src/test/ui/proc-macro/macro-rules-derive-cfg.rs
    [ui] src/test/ui/proc-macro/nested-nonterminal-tokens.rs
    [ui] src/test/ui/proc-macro/nodelim-groups.rs
    [ui] src/test/ui/proc-macro/nonterminal-expansion.rs
    [ui] src/test/ui/rfc-2294-if-let-guard/feature-gate.rs
    [ui] src/test/ui/rfc-2361-dbg-macro/dbg-macro-expected-behavior.rs
    [ui] src/test/ui/type/ascription/issue-47666.rs
    [ui] src/test/ui/unsafe/ranged_ints_macro.rs#mir
    [ui] src/test/ui/unsafe/ranged_ints_macro.rs#thir

[nnethercote]

Interesting: the change of pretty-printing from "foo::bar" to "foo :: bar" broke some links in doctests. Fortunately it's pretty easy to address, just by adding ModSep to tt_prepend_space.

[nnethercote]

I have updated the code, which has a bunch of fixes, including a couple from @petrochenkov's branches. I have disabled two tests and commented out some failing-to-parse assertions in one unit test. Except for those, all tests now pass, so that's some kind of progress. Still plenty of wrinkles to iron out, though, and who knows what a crater run might uncover.

[bors]

☔ The latest upstream changes (presumably #97239) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #97293) made this pull request unmergeable. Please resolve the merge conflicts.

[Dylan-DPC]

@nnethercote what's the update on this?

[nnethercote]

I'm not planning to work on it further, because it seems too hard and likely to have major backwards incompatibility problems. I am ok with closing it, if you want.

[Dylan-DPC]

Thanks for the update. Closing it then :)