[CS2] Fix #4467: tagged template literal call #4601
Conversation
| o 'This' | ||
| o 'Super' | ||
| o 'Super', -> new Value $1 |
There was a problem hiding this comment.
this is the same grammar change from #4592 (to allow eg super.x.y), all the rest of the changes here in grammar.coffee are related to making Invocation a grammar Value
| @@ -266,7 +266,8 @@ exports.Base = class Base | |||
| # For this node and all descendents, set the location data to `locationData` | |||
| # if the location data is not already set. | |||
| updateLocationDataIfMissing: (locationData) -> | |||
| return this if @locationData | |||
| return this if @locationData and not @forceUpdateLocation | |||
There was a problem hiding this comment.
this @forceUpdateLocation is the workaround I described in the PR comment - it gets set inside Value.add() below to allow the subsequent call to updateLocationDataIfMissing() with (seemingly correct) updated location data that includes the location of the added property to flow through and set the Value's locationData. This was the cleanest way I could see to allow updating the locationData for this one case
| @@ -2635,7 +2637,8 @@ exports.Op = class Op extends Base | |||
| if op is 'do' | |||
| return Op::generateDo first | |||
| if op is 'new' | |||
| return first.newInstance() if first instanceof Call and not first.do and not first.isNew | |||
| if (firstCall = first.unwrap()) instanceof Call and not firstCall.do and not firstCall.isNew | |||
There was a problem hiding this comment.
this and the two other below changes to nodes.coffee are places where a Call that's now wrapped in a Value (due to the grammar changes) needed to be unwrapped in order for tests to pass
|
|
||
| test 'Values with properties end up with a location that includes the properties', -> | ||
| source = ''' | ||
| a.b |
There was a problem hiding this comment.
In addition to this test, perhaps another that has a bit more code around it? Perhaps with new? So that we have more confidence that the location data is correct.
There was a problem hiding this comment.
@GeoffreyBooth I'd say the new case is already covered by the new calls have a range including the new test in test/parser.coffee that led me to this bug, but I added more cases here that test multiple accesses/indexes
|
LGTM. @lydell? |
|
LGTM |
Fixes #4467
I came to this issue from the other direction - I'd noticed in the grammar that
Invocation(ie a function call) really ought to be a grammarValue- it simplifies things grammar-wise and makes intuitive sense. The one thing that I saw that that made legal (ie where previously you could use aValuebut not anInvocation) was anInvocationas the "function" of aTaggedTemplateLiteral- so #4467 having been opened about that specific case validates my thinkingBy making
Invocationa grammarValue, itsCallgets wrapped in anew Value()in places where it previously would've been an unwrappedCall(ega().b). So mostly then had to go through and add some.unwrap()s innodes.coffeewhere things were expecting an unwrappedCallbut could now be getting aCallwrapped in aValue. There could be other places innodes.coffeethat should be updated in this way, I mostly just went through and found the places that were breaking testsOne interesting test breakage was the test in
test/parser.coffeethat checks thelocationDataofa = new B().c(d). This led me through #619 which explains that the AST fornewexpressions like this aren't actually correct (ie don't match JS' interpretation, just generate the correct compiled JS). But the actual bug ended up being that the location data forValues with properties don't include the properties. The test was previously passing because in the grammar ruleo 'Invocation Accessor', -> new Value $1, [].concat $2, thenew Value()is initialized with its property. But with the grammar change, the property was being.add()ed to theValuelater. So this basically just exposed the bug that typicalValues with properties don't include the properties' locations in their location data. From my understanding this is because oncelocationDatais set (at the time the grammar rule creating thenew Value()is run),updateLocationDataIfMissing()will then ignore the subsequent (correct, expanded) location data that gets sent when the ruleo 'Value Accessor', -> $1.add $2gets run. So I added a workaround for this specific case of.add()ing properties toValuesThere are a couple other little
nodes.coffeecleanups included, I'll make some comments on the code @GeoffreyBooth