AST Improvements

[MichaReiser]

We integrated RustPython's AST into Ruff last week. This makes it easier for us to make changes to the AST that better suit the needs of static analysis (Python compatibility is a non-goal) or make it more approachable.

I want to use this discussion to propose a few improvements that we can implement independently if we agree.

[MichaReiser]

Naming

Avoid abbreviations

Compiler terminology can be hard to approach, even more so if the code heavily uses abbreviations. Our AST uses plenty of them

• Expr instead of Expression
• Stmt instead of Statement
• StmtClassDef instead of StmtClassDefinition
• Mod instead of Module
• CmpOp -> ComparisonOperation
• UnaryOp -> UnaryOperation
• elts -> elements
• ops -> operators
• orelse -> or_else (not an abbreviation per se)

I propose that we write out all names in full. Doing so also fixes inconsistency in our AST. For example, we already use Pattern and not Ptrn and ExceptHandler and not ExHn. The same applies to all fields.

The main disadvantage is that things no need a few keystrokes more to write out, but IDEs are pretty good add helping us with that.

Type postfix

Our AST uses a prefix notation to denote to which kind a node belongs: Stmt<Node> for all statements, Expr<Node> for all expressions. I propose to use a postfix notation instead, because it reads more naturally: IfStatement rather than StmtIf (which sounds like it only is a condition if`). Examples:

• StmtFunctionDef -> FunctionDefinitionStatement
• StmtIf -> IfStatement
• PatternMatchSequence -> MatchSequencePattern
• ExprUnary
• ExprBoolOp -> BooleanOperationExpression

Any Prefix

We already have some precedence of this pattern in our code base today; much of it is my fault. The proposal is to prefix node-unions with Any:

• AnyExpression
• AnyStatement
• AnyPattern
• AnyFunctionDefinitionStatement
• ...

This has two motivation

• It avoids name conflicts: E.g. what to name the enum that represents FunctionDefintionStatement and AsyncFunctionDefinitionStatement? The obvious choice is FunctionDefinitionStatement with a Sync and Async variant, but that name's already taken (arguably, it could make sense to merge those two nodes...)
• The name communicates to you whether you deal with a single Node or a selection of nodes.

You could argue that the IDE provides you with that information and that the argument is similar to proposing annotating your variables with their type: upper_u32. I agree that this not strictly necessary, but I found it useful for the above-mentioned points

Variant Names / Is-methods

I like the current naming of enum variants that removes the kind-prefix (postfix in the future): Stmt::If instead of Stmt::Stmt::If(stmt). I like it because the Statement in the variant is redundant, and I often ran into situations at Rome where I imported the variant name instead of the node type.

However, the naming is inconsistent with the is methods that are explicitly annotated. It's:

• stmt.is_if_stmt() and not stmt.is_if()
• stmt.if_stmt() and not stmt.if() (I still think that it should be as_if() but we would need to fork the is macro
• stmt.into_if_stmt() and not stmt.into_if()

My recommendation is to align the naming and I'm leaning more towards removing the prefix/postfix.

Renaming Nodes

• JoinedString: Searching for Python joined string returns no useful result (on the first page) and I remember that it took me an awful long time to figure out what this node represents. I don't know why they are named that way in the Python AST, because joined isn't mentioned once in PEP 498. I would probably go with FString or FormattedString, this seems to be the established notion.
• ExprIfExp: This one is confusing because it contains Expression twice. IfExpression should be sufficient (and is different from IfStatement)
• Rename Constant* to Literal*. While it's true that these values are Constants at runtime (and is why these names make sense for an interpreter), they are literals in the source code (which we analyze). The python documentation also seems to use the term Literal

[MichaReiser]

Structural changes

Implicitly concatenated strings

It is important to know during formatting whether a string is implicitly concatenated or not, but the AST doesn't expose this information and the formatter has to iterate over the whole string.

I recommend that we include this information in the AST and remove the parser's automatic joining of strings with implicit concatenation into a single String constant.

• Introduce a new StringConstant enum with the String(String) and Concatenated(Vec<String>)
• Implement helpers on StringConstant
  • as_slice (use std::slice::from_ref)
  • iter to iterate over all parts,
  • to_string (returns the concatenated String),
  • is_implicit_concatenated

Open questions:

• Implicit concatenation is also a thing for f-strings and byte strings. We should come up with a uniform design. The main challenge is that it must be a single Expression node
• Should StringConstant's PartialEq return true if the joined strings are equal? "ABC" == "A" "B" "C"

Alternatives:

• Add an is_implicit_concatenated field to the node, and add a helper method to split the string into its parts.

Async / Sync variants

There are multiple nodes with a sync/async variant. I recommend merging them and instead adding a async: bool to distinguish between sync and async.

When building interpreters, distinguishing between a sync and async function makes sense because they have fundamentally different execution models. However, we almost always treat them the same during linting and formatting. We've created Any<Node> unions to get a unified API, but I think we can just fix it in the AST instead.

FormattedValue

Remove Conversion::None and instead default to Conversion::Str because that's the default if no explicit conversion is specified (It defaults to Repr for debug).

The goal of the AST is to capture the semantical meaning of the code, not its precise source. That's why unifying None and Str is sufficient. A CST, on the other hand, would want to preserve all variants.

Introduce Suite node

Create a node type for Vec<Stmt> instead of using a type alias.

TODO: Motivation

[MichaReiser]

Remove unsupported fields & nodes

Remove the following fields and nodes that are not supported by the parser:

• TypeIgnore
• type_comment
• ModFunctionType
• ModInteractive

[MichaReiser]

Done

[charliermarsh]

This is awesome!

[charliermarsh]

I may do "Async / Sync variants" quickly for fun, I need something light to work on and this bothers me.