Rust-analyzer as library: potential overlap between `expr()/tail_expr()` and `statements()` in `ast::MacroStmts` and `ast::StmtList`

[redsun82]

rust-analyzer version: 0.0.300

As far as I understand generated getters in ast classes take the first child that matches the required type. However since AsmExpr is now also an Item (and therefore a Stmt), this can lead to overlaps where the last element of ast::MacroStmts::statements() will be a duplicate of ast::MacroStmts::expr(). This can be reproduced by any use of asm!, for example this one coming from https://doc.rust-lang.org/reference/inline-assembly.html

code snippet to reproduce:

use std::arch::asm;

let mut x: u64 = 4;
unsafe {
    asm!(
        "mov {tmp}, {x}",
        "shl {tmp}, 1",
        "shl {x}, 2",
        "add {x}, {tmp}",
        x = inout(reg) x,
        tmp = out(reg) _,
    );
}

When expanding macros, this will result in a MacroStmts instance that exhibits this duplication. I don't know if this could cause any issues elsewhere in the library.

Theoretically this problem could also appear in StmtList, which is

  '{'
    Attr*
    statements:Stmt*
    tail_expr:Expr?
  '}'

but I think that can't happen in practice yet, as there's no other way to produce an AsmExpr (and therefore something that is both an Expr and a Stmt) than by asm! or similar inline assembly macros, so an AsmExpr won't show up as a tail_expr in any StmtList instance.

[redsun82]

As context, as far as I can tell this is akin to the issue that brought about this code in ast::PathSegment instead of relying on generated ast::TypeAnchor::type() and ast::TypeAnchor::path_type().

[ChayimFriedman2]

I don't see this as a problem. Do you have a concrete use-case where this causes issues?

[redsun82]

We use rust-analyzer as a library on top of which we build CodeQL support for rust (https://github.com/github/codeql). This has caused a problem when we updated to 0.0.300. While we did manage to put a workaround in place, it seems to me:

• child getters in an AST library shouldn't be ambiguous, and should reflect precisely what the parser saw
• this seems like an instance of spooky action at a distance
• I do fear new instances of this (ambiguous getters caused by changes in the AST hierarchy) could pop up again in the future
• I also have the feeling that this has the potential to pop up as a bug in some other part of rust-analyzer relying on generated ast getters (although admittedly this might not be the case in this specific instance)

[ChayimFriedman2]

The usual solution will be to wrap one of the AsmExprs in its own node (probably the item one). I don't mind accepting a fix if this is problematic for you.