Distinguish property access in AST #14226
Conversation
kazcw
added
the
CI: No changelog needed
kazcw
requested review from
4e6,
AdRiley,
Akirathan,
Frizi,
JaroslavTulach,
PabloBuchu,
farmaazon,
hubertp,
indiv0,
somebody1234 and
vitvakatu
as code owners
| @Test | ||
| public void dotUnderscore() throws Exception { | ||
| var ir = | ||
| parse( | ||
| """ | ||
| run op = | ||
| op._ | ||
| """); | ||
|
|
||
| assertSingleSyntaxError(ir, Syntax.InvalidUnderscore$.MODULE$, "Invalid use of _", 14, 15); | ||
| } | ||
|
|
||
| @Test | ||
| public void spaceDotUnderscore() throws Exception { | ||
| var ir = | ||
| parse( | ||
| """ | ||
| run op = | ||
| op ._ | ||
| """); | ||
|
|
||
| assertSingleSyntaxError( | ||
| ir, Syntax.UnexpectedExpression$.MODULE$, "Unexpected expression", 14, 16); | ||
| } | ||
|
|
||
| @Test | ||
| public void dotUnderscore2() throws Exception { | ||
| var ir = | ||
| parse( | ||
| """ | ||
| run op = | ||
| op._.something | ||
| """); | ||
|
|
||
| assertSingleSyntaxError(ir, Syntax.InvalidUnderscore$.MODULE$, "Invalid use of _", 14, 15); | ||
| } | ||
|
|
There was a problem hiding this comment.
I have removed some tests that only check for detection of syntax errors, as these are now covered by the parser's tests.
There was a problem hiding this comment.
- Shouldn't you just update the error message?
- There should still be
assertSingleSyntaxErrorin such test, right? - Why not keep those tests?
| if (t instanceof Tree.Wildcard wild) { | ||
| return join(new Name.Blank(getIdentifiedLocation(wild.getToken(), generateId), meta()), nil()); | ||
| } | ||
| List<Name> names = nil(); | ||
| while (t instanceof Tree.PropertyAccess app) { | ||
| names = join(sanitizeName(buildName(app.getRhs(), generateId)), names); | ||
| t = app.getLhs(); | ||
| } | ||
| if (t instanceof Tree.Ident id) { | ||
| names = join(sanitizeName(buildName(id, generateId)), names); | ||
| } else { | ||
| throw translateEntity(t, "qualifiedNameSegment"); |
There was a problem hiding this comment.
This is a bit stricter than the original logic: A blank (_) on its own is (still) allowed, but a blank as the first-segment of a multi-segment name is no longer allowed. I don't think the later case makes sense and I expect that attempting to use it would have previously yielded semantic errors; it is now caught here.
| test.each([ | ||
| { target: 'a.b', pattern: '__', extracted: ['a.b'] }, | ||
| { target: 'a.b', pattern: 'a.__', extracted: ['b'] }, | ||
| { target: 'a.b', pattern: 'a.__', extracted: [] }, |
There was a problem hiding this comment.
This is no longer expected to match, as the Pattern API only matches expressions, and the RHS of the operator is now just a token. I have checked all usages of the Pattern API to make sure we were not relying on the previous behaviour in this type of case.
| && isDotOperator(oprApp.getOpr().getRight()) | ||
| && oprApp.getRhs() instanceof Tree.Ident) { | ||
| func = translateExpression(oprApp.getRhs(), true); | ||
| if (tree instanceof Tree.PropertyAccess oprApp) { |
There was a problem hiding this comment.
This looks like simplification.
| /** | ||
| * Substitute `pattern` inside `expression` with `to`. | ||
| * Will only replace the first item in the property acccess chain. | ||
| * Substitute `pattern` in any `Ident` nodes inside `expression` with `to`. |
There was a problem hiding this comment.
I would add a warning/remainder that right side of PropertyAccess is not Ident node.
| get operator(): Token { | ||
| return this.module.getToken(this.fields.get('operator').node) | ||
| } | ||
| /** TODO: Add docs */ |
There was a problem hiding this comment.
Let's add one or two docs on this occasion.
| } else { | ||
| match self.pop_macro_stack_if_reserved(repr) { | ||
| Some(popped) => { | ||
| self.resolve(popped); | ||
| return Step::MacroStackPop(token.into()); | ||
| } | ||
| _ => {} | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't feel it is better now (if let expresses the thing more concisely), but perhaps it's a matter of taste new edition?
| test.each([ | ||
| { target: 'a.b', pattern: '__', extracted: ['a.b'] }, | ||
| { target: 'a.b', pattern: 'a.__', extracted: ['b'] }, | ||
| { target: 'a.b', pattern: 'a.__' }, |
There was a problem hiding this comment.
I guess the AST patterns are able to catch/substitute AST nodes only, not tokens inside a node? I would either add comment here or just remove this case.
Pull Request Description
Add a
PropertyAccessnode to the Rust AST, representing use of the dot.operator. This operator was previously treated as a special case both in the GUI and in the backend.The
PropertyAccessnode only represents syntactically-valid applications of the operator. Its RHS is non-optional, and is an identifier token, not an expression.This eliminates the largest difference between AST and Tree, and so is a major step toward #10754.
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.