Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Expr, Stmt, Pat to schema.py #17390

Merged
merged 13 commits into from
Sep 9, 2024
Merged

Conversation

aibaars
Copy link
Contributor

@aibaars aibaars commented Sep 5, 2024

This PR adds classes for expressions, statements, and patterns to the schema. The code is most auto-generated by co-pilot. The definitions of the HIR datatypes can be found at https://github.com/rust-lang/rust-analyzer/blob/master/crates/hir-def/src/hir.rs#L169-L300

@paldepind
Copy link
Contributor

Looks amazing! 🎉 Having AstNode would also be very useful I think. I'm looking at setting up the boilerplate for the CFG and AstNode is used a bunch of places.

Copy link
Contributor

@redsun82 redsun82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small naming nits and one probable typo, and some questions from my side, but this looks great as a starting point, thanks!

We should not forget when we do start extracting things to add some code snippets as examples to class docstrings. That helps us along the way, the QL writer then, and if we don't do it from the start we risk never really catching up

rust/schema.py Outdated Show resolved Hide resolved
rust/schema.py Outdated
# },


class Async(Block):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would maybe AsyncBlock be more informative on the QL side?

rust/schema.py Outdated Show resolved Hide resolved
class Call(Expr):
callee: Expr
args: list[Expr]
is_assignee_expr: predicate
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uh, this is making me curious 🙂

# },


class Yeet(Expr):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the curious, this is an experimental feature that we're not probably going to really encounter https://doc.rust-lang.org/stable/unstable-book/language-features/yeet-expr.html

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should just remove it for now? Given that it's highly experimental and placeholder syntax that is going to change in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. I could remove the class and translate yeet to MissingExpr in the extractor.

rust/schema.py Outdated Show resolved Hide resolved
class BinaryOp(Expr):
lhs: Expr
rhs: Expr
op: optional[string]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of curiosity, do you know what does op: None mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea, I am equally curious. Just haven't found the time to run an example expression through the pretty_print function.

@aibaars aibaars marked this pull request as ready for review September 6, 2024 09:52
@aibaars aibaars requested a review from a team as a code owner September 7, 2024 08:33
@aibaars aibaars added the no-change-note-required This PR does not need a change note label Sep 9, 2024
@aibaars aibaars merged commit ef34d24 into rust-experiment Sep 9, 2024
8 of 9 checks passed
@aibaars aibaars deleted the aibaars/rust-experiment branch September 9, 2024 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants