-
Notifications
You must be signed in to change notification settings - Fork 34
Add Ghost Dot Syntax #142
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
base: main
Are you sure you want to change the base?
Add Ghost Dot Syntax #142
Conversation
CatarinaGamboa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple changes
| } | ||
|
|
||
| private Expression dotCallCreate(DotCallContext rc) throws LJError { | ||
| if (rc.OBJECT_TYPE() != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are a couple more cases here right?
OBJECT_TYPE actually accepts multiple fields like this.a.b so there could be several indexes of . maybe we can send an error if that happens since we dont allow refering to field chains (yet at least) but we shouldn't just ignore it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep!
| String simpleName = text.substring(dot + 1); | ||
| String name = Utils.qualifyName(prefix, simpleName); | ||
| List<Expression> args = getArgs(rc.args(0)); | ||
| if (!args.isEmpty() && args.get(0)instanceof Var v && v.getName().equals(Keys.THIS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
picky space betwee args.get(0) and instanceof
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't like it, blame the formatter 😂
| } | ||
| } | ||
|
|
||
| private Expression dotCallCreate(DotCallContext rc) throws LJError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We def need documentation here explaining what the function does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I add documentation for all other functions in this file then?
| throw new SyntaxError("Cannot use both dot notation and explicit 'this' argument. Use either 'this." | ||
| + simpleName + "()' or '" + simpleName + "(this)'", text); | ||
|
|
||
| args.add(0, new Var(target)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comment here saying this transforms the call this.func to be used internally as func(this)
| List<Expression> targetArgs = getArgs(rc.args(0)); | ||
| List<Expression> funcArgs = getArgs(rc.args(1)); | ||
| funcArgs.add(0, new FunctionInvocation(targetFunc, targetArgs)); | ||
| return new FunctionInvocation(name, funcArgs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also add comment here saying what the transformation is
This PR adds support for dot syntax in ghost variables and states while keeping it backward compatible with the previous syntax. For this, a new non-terminal
dotCallwas added to the grammar and implemented in the AST visitor.Currently, all of these are equivalent:
size(this),this.size(), andsize(). To refer to the previous value ofsize, we can either usesize(old(this))orold(this).size.In the future, we plan to drop the functional syntax and only keep the dot syntax to avoid ambiguity and confusion.
Example