[WIP] Remove ids from ast::Stmt and hir::Stmt

[Aaron1011]

cc #87779 #87969

r? @ghost

[Aaron1011]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 81925c43af54c8277b50c63cdbd6934d96852484 with merge a52a6ff6458c7e403c0688534766d0eb58d707ca...

[Aaron1011]

@bors try

[bors]

⌛ Trying commit 6aeceafcd6a04a0408d98233f42452f1f7b73782 with merge 7f2b8ca23dea09d75147052e29614a6e39bbe441...

[Aaron1011]

@bors try

[bors]

⌛ Trying commit 741210c0c4b79c55040baaaeeb1d3d4f073add09 with merge 714cf7d474e2f80a32ce3db9b014e24cb0834aa2...

[bors]

☀️ Try build successful - checks-actions
Build commit: 714cf7d474e2f80a32ce3db9b014e24cb0834aa2 (714cf7d474e2f80a32ce3db9b014e24cb0834aa2)

[rust-timer]

Queued 714cf7d474e2f80a32ce3db9b014e24cb0834aa2 with parent 4498e30, future comparison URL.

[rust-timer]

Finished benchmarking try commit (714cf7d474e2f80a32ce3db9b014e24cb0834aa2): comparison url.

Summary: This change led to significant mixed results 🤷 in compiler performance.

• Moderate improvement in instruction counts (up to -0.7% on incr-unchanged builds of keccak)
• Large regression in instruction counts (up to 2.3% on incr-full builds of clap-rs)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

[bors]

⌛ Trying commit a404da8 with merge 2e7172f5abd1fb24fdc05fb9553907214b17592e...

[bors]

☀️ Try build successful - checks-actions
Build commit: 2e7172f5abd1fb24fdc05fb9553907214b17592e (2e7172f5abd1fb24fdc05fb9553907214b17592e)

[rust-timer]

Queued 2e7172f5abd1fb24fdc05fb9553907214b17592e with parent 0fa3190, future comparison URL.

[rust-timer]

Finished benchmarking try commit (2e7172f5abd1fb24fdc05fb9553907214b17592e): comparison url.

Summary: This change led to significant mixed results 🤷 in compiler performance.

• Small improvement in instruction counts (up to -0.8% on incr-full builds of keccak)
• Moderate regression in instruction counts (up to 1.7% on incr-full builds of clap-rs)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

[Aaron1011]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit cd8eedf with merge 83a83392e6fb744ae3f64cb43aafc8ea08e88059...

[bors]

☀️ Try build successful - checks-actions
Build commit: 83a83392e6fb744ae3f64cb43aafc8ea08e88059 (83a83392e6fb744ae3f64cb43aafc8ea08e88059)

[rust-timer]

Queued 83a83392e6fb744ae3f64cb43aafc8ea08e88059 with parent 2fc3c69, future comparison URL.

[rust-timer]

Finished benchmarking try commit (83a83392e6fb744ae3f64cb43aafc8ea08e88059): comparison url.

Summary: This change led to significant mixed results 🤷 in compiler performance.

• Small improvement in instruction counts (up to -1.3% on incr-patched: println builds of coercions)
• Moderate regression in instruction counts (up to 1.7% on incr-full builds of clap-rs)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

[Aaron1011]

I'm not certain if we should do this.

Pros:

• Simplification of AST and macro expansion - we don't need to have weird handling of statement ids in noop_flat_map_stmt or during ID assignment.
• Makes AST statements more like a thin wrapper over another AST node, as they now 'inherit' everything from the contained node.
• Potential for (small) memory savings when compiling crates with a large number of statements.

Cons:

• In the HIR, we do actually need to distinguish between statements and their inner HIR node during scope tree building. We need to be able to create 'outer' scope(s) for the statement itself, and 'inner' scopes for the contained local/expression. This is necessary to be able to continue to drop things like (a, &b, c, &d) in the correct order - a single scope is not enough to describe the drop order.
• Retrieving statement spans is somewhat awkward - we need to get the parent block, and then find the statement containing the target item/expression/local.
• It's conceivable that in the future, we'll add another usage of HIR that needs to distinguish between statements and the inner AST node, similar to scope tree building. Giving an HIR ID to statements makes this easy- otherwise, we'll need another ad-hoc solution similar to what I did for scope trees.
• This might make things more difficult for clippy going forward (there are several test failures that I haven't yet investigated).

Originally, I had hoped to do this just for the AST, to remove the special cases around statement NodeId handling. Unfortunately, that just ended up requiring a different special case - we would need to synthesize NodeIds during lowering to HIR, instead ofassigning them ahead of time.

[petrochenkov]

Ok, if HIR processing logic wants a separate ID for statements, then let's have a separate ID for HIR statements, with that ID removed this PR doesn't look like a simplification at all.

What I don't understand is why #87779 didn't "just work" (perhaps with minor modifications).
We have a bunch of nodes with IDs which have other nested nodes with IDs, and the generic ID assignment logic covers them all. Then why are statements special?
(I have some time so I'll try to experiment with expand-only changes today.)

[petrochenkov]

Ok, now I understand why IDs have to be assigned to statements during placeholder substitution.
Without that in the StmtKind::Item(ItemKind::MacCall) case we create a single statement, assume that it is "final" and is going to expanded AST, and assign an ID to it. The ItemKind::MacCall in the meantime gets expanded to multiple items (as in #87877) and eliminates our supposedly "final" statement as well, breaking things in process.

Note that the same thing doesn't happen with attribute invocations in StmtKind::Item and other kinds.
InvocationCollector::flat_map_stmt always pulls out attributes from the StmtKind nodes and treats them as statement attribute invocations, and not as item attribute invocations etc.

I think we should do the same thing with fn-like macro calls - pull out ItemKind::MacCalls from item statements in InvocationCollector::flat_map_stmt and treat them as statement macro calls.

Besides the consistency with attributes, this is also consistent with the token-based expansion model (in which we are reparsing all nonterminals from tokens and AST pieces in token::Nonterminal are only used as a "cache" for performance).
In this model the example from #87877 expands like this:

macro_rules! two_items {
    () => {
        extern "C" {}
        extern "C" {}
    };
}

macro_rules! single_item_funneler {
    ($item:item) => {
        $item
    };
}

fn main() {
    single_item_funneler! { two_items! {} }
}

=>

macro_rules! two_items {
    () => {
        extern "C" {}
        extern "C" {}
    };
}

fn main() {
    ⟪
        two_items! {}
    ⟫
}

=>

fn main() {
    extern "C" {}
    extern "C" {}
}

, in other words, two_items! {} is reparsed as a statement macro call despite being originally matched as item.

I'll try to implement this scheme today.