-
Notifications
You must be signed in to change notification settings - Fork 2k
Preserve aliased operators #5059
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
Preserve aliased operators #5059
Conversation
|
A couple other things that ultimately will need to be preserved:
As well as things like whether the postfix form of |
GeoffreyBooth
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.
So the general idea of using String objects to sneak extra properties through the lexer is very clever. This is another workaround along the lines of my #5045 PR, but a better approach than that PR. That said, it’s awfully annoying to work with String objects as opposed to primitive strings. Could we perhaps add the extra properties onto the token objects while we’re working in the lexer, and only shift them onto this new String object at the last second before sending the data through the parser? Perhaps we could add one final new rewriter pass, that looks for extra properties on tokens and creates String objects to move them onto? And then likewise on the nodes side, perhaps the constructor of the Base class could check if value was a String object rather than a primitive string and if so, add the properties to the class and convert value back into a primitive string.
src/helpers.coffee
Outdated
| else string | ||
|
|
||
| exports.normalizeStringObject = (str) -> | ||
| str?.valueOf?() ? str |
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 think we can do without this function. Everywhere you’re currently calling it, you could do
"#{str}"instead. Proof:
primitiveString = 'foo'
stringObject = new String 'foo'
primitiveString is stringObject # false
primitiveString is "#{stringObject}" # trueb3f4828 to
429ab6a
Compare
|
@GeoffreyBooth merged |
GeoffreyBooth
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.
Looks pretty good, just a few minor changes.
src/grammar.coffee
Outdated
|
|
||
| Operation: [ | ||
| o 'UNARY Expression', -> new Op $1 , $2 | ||
| o 'UNARY Expression', -> new Op $1.toString(), $2, null, null, originalOperator: $1.original |
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.
Rather than passing null to skip arguments, we should pass undefined, as a general rule. Since only undefined allows function parameter default values to be set.
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.
updated to undefined (in all the updated rules)
src/nodes.coffee
Outdated
| @operator in ['<', '>', '>=', '<=', '===', '!=='] | ||
|
|
||
| invert: -> | ||
| if @isIn() |
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.
Until I read farther down, I thought this meant if is inverted.
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.
Ya this is a tricky name - updated to @isInExpression() but even that could be misinterpreted. @isAnInExpression(), though less ambiguous, looks weird to me
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.
Well doesn’t in refer to the operator? And this is in the Op class, so shouldn’t we call this isInOperator?
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.
@GeoffreyBooth sure updated to isInOperator()
helixbass
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.
@GeoffreyBooth updated per your comments
src/nodes.coffee
Outdated
| @operator in ['<', '>', '>=', '<=', '===', '!=='] | ||
|
|
||
| invert: -> | ||
| if @isIn() |
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.
Ya this is a tricky name - updated to @isInExpression() but even that could be misinterpreted. @isAnInExpression(), though less ambiguous, looks weird to me
src/grammar.coffee
Outdated
|
|
||
| Operation: [ | ||
| o 'UNARY Expression', -> new Op $1 , $2 | ||
| o 'UNARY Expression', -> new Op $1.toString(), $2, null, null, originalOperator: $1.original |
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.
updated to undefined (in all the updated rules)
@GeoffreyBooth @zdenko another WIP similar to #5057: this one preserves the original operator eg
on/yes/trueoris/==Uses the same technique of stashing
.originalon anew String()wrapper around the token value. While I followed the same pattern in the node constructor of immediately callingnormalizeStringObject()to unwrap to a primitive string, I ran into more cases withinlexer.coffeeitself where it wanted to do direct string comparisons against the token value so I had to callnormalizeStringObject()in other places where it could be wanting to compare to a wrappedString()I've wired this one up fully with Prettier/AST and am able to see the original operator in reformatted source
The only other thing I was thinking of including in this PR was preserving
@vsthis? And I guess::vs.prototype