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

Representation for assignee expressions in assignment expressions #35

Open
xFrednet opened this issue Feb 23, 2023 · 1 comment
Open
Labels
A-stable-api Area: Stable API, How it should look and what should be included in it C-discussion

Comments

@xFrednet
Copy link
Member

The reference defines assignment expressions to have the grammar:

AssignmentExpression:
Expression = Expression

which makes sense. However, it becomes slightly more complicated when the assignee expression is an array, a tuple or a struct with fields, as these can contain wildcard/underscore and rest expressions (as shown below and on the Playground).

a += 1;
[a, b] = [1, 2];
S {
    array: [_, a, ..],
    slice: (b, ..),
} = S::default();

The wildcard/underscore expression is AFAIK only used to allow such assignments. The rest expression, is not listed and on a lexicographical level probably a range expression without limits.

While this works, it feels weird to me. When I think about assignments, I see the left-hand side as a pattern. Adding a wildcard/underscore expression just for assignments also doesn't feel quite right.

Rustc desugars such expressions into a let-statement with a pattern and variables that are then reassigned. This can be seen with Show HIR on the Playground.


The question of this issue is, how should the assignee be represented in Marker?

  1. With expressions, like the grammar would indicate
  2. With patterns and some way of including place expressions in patterns
@xFrednet xFrednet added A-stable-api Area: Stable API, How it should look and what should be included in it C-discussion labels Feb 23, 2023
@xFrednet
Copy link
Member Author

I've played around with both options and currently prefer the second one. The pattern nodes are already used for let-statements, using them would make the representation of the assign expression be similar. Helper functions for let-statements can then potentially be reused by assign expressions. I also believe that it'll be simpler to document the API if patterns are always used as assignees.

Representing it as a pattern is also closer to my mental model, but that is not really a good argument.

I've tried implementing the first version in the branch 052-expr-as-assign-target but stopped half way, as it didn't feel quite right. I've implemented the second option in 052-assign-exprs which seems to work well. I'll create a PR for the second solution to see how it feels over time.

This has to be discussed before v1.0.0, even if we agree that the current version is working well. Any feedback is as always welcome :)

bors bot added a commit to rust-marker/marker that referenced this issue Feb 25, 2023
118: Add AssignExpr API represention r=Niki4tap a=xFrednet

This PR adds the assign expression to the API. The implementation was a bit complicated design wise, I created rust-marker/design#35 to discuss other solutions and review this representation before v1.0.0.

Before reviewing this, I suggest checking this [Playground](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=0da87ed83a1f80c61b026f54bf75d077) with *Show HIR* to see how rustc desugars some assign expressions. `#[clippy::dump]` and then *Tools > Clippy* can also always be helpful.

I also appreciate feedback on the design.

---

r? `@Niki4tap` Would you mind giving this a review? We can also wait a few days to see if we get any feedback on the design issue. On the other hand, we can always revert these changes :). Besides that, I'm trying to document all new nodes added to the API, we'll see how that goes ^^.

#52 

Co-authored-by: xFrednet <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stable-api Area: Stable API, How it should look and what should be included in it C-discussion
Projects
None yet
Development

No branches or pull requests

1 participant