Closed Bug 1378808 Opened 7 years ago Closed 6 years ago

Column Breakpoints should be available for member expression

Categories

(DevTools :: Debugger, defect, P3)

Product:
DevTools
Component:
Debugger
Type:
defect

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: jlast, Assigned: jorendorff)

References

Details

Attachments

(4 files)

Bug 1378808 - Add a new ParseNodeKind::Arguments node type for call argument lists.
59 bytes, text/x-review-board-request
jorendorff
: review+
Details
Bug 1378808 - Add a new ParseNodeKind::PropertyName to hold location information about property access name.
59 bytes, text/x-review-board-request
jorendorff
: review+
Details
Bug 1378808 - Use ::Arguments or ::PropertyName location for method call column offsets.
59 bytes, text/x-review-board-request
jorendorff
: review+
Details
Part 4: Adjust web-platform-test to allow more precise error location
1.91 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review 
Users can currently add column breakpoints at call sites like a(b(), c()), 
but they cannot add breakpoints at member expressions like a().b().c().

The difference is that, in the first example the engine assigns 
different start locations to "a", "b", and "c". In the second example, they 
all have the same location.

This is easy to reproduce with `script.getAllColumnOffsets()`.
Priority: -- → P3
Here is the patch we had before:

diff --git a/js/src/frontend/BytecodeEmitter.cpp b/js/src/frontend/BytecodeEmitter.cpp
index a45bfc6..d7212a0 100644
--- a/js/src/frontend/BytecodeEmitter.cpp
+++ b/js/src/frontend/BytecodeEmitter.cpp
@@ -4111,6 +4111,9 @@ BytecodeEmitter::emitPropOp(ParseNode* pn, JSOp op)
     if (op == JSOP_CALLPROP && !emit1(JSOP_DUP))
         return false;

+    if (!updateSourceCoordNotes(pn->pn_pos.end))
+        return false;
+
     if (!emitAtomOp(pn, op))
         return false;

@@ -4987,6 +4990,9 @@ BytecodeEmitter::emitScript(ParseNode* body)
             return false;
     }

+    if (!updateSourceCoordNotes(body->pn_pos.end))
+        return false;
+
     if (!emit1(JSOP_RETRVAL))
         return false;

@@ -9660,11 +9666,11 @@ BytecodeEmitter::emitCallOrNew(ParseNode* pn, ValueUsage valueUsage /* = ValueUs

     if (!spread) {
         if (pn->getOp() == JSOP_CALL && valueUsage == ValueUsage::IgnoreValue) {
-            if (!emitCall(JSOP_CALL_IGNORES_RV, argc, pn))
+            if (!emitCall(JSOP_CALL_IGNORES_RV, argc, nullptr))
                 return false;
             checkTypeSet(JSOP_CALL_IGNORES_RV);
         } else {
-            if (!emitCall(pn->getOp(), argc, pn))
+            if (!emitCall(pn->getOp(), argc, nullptr))
                 return false;
             checkTypeSet(pn->getOp());
         }
Blocks: js-devtools
Product: Firefox → DevTools
I've been looking into this for jlast a bit. I've included my current patch at the bottom. There are three issues/questions that have come up that I wanted feedback on:

1. By using `pn2->pn_pos.end`, we're placing the breakpoint immediately after the callee. While that mostly works, it's not actually guaranteed to be the location of the `(` in the call, since there may be comments or whitespace separating the callee from the `(`. I can't see this information represented in the AST anywhere. Is there precedent for representing that?

2. It also seems like ideally we'd try to put the column at the start of properties instead of the `(`, for things like `obj.foo()` placing the column on the `f`, but the `name` doesn't appear to have location information. Is that worth adding?

3. Now the most important question that has come up is that the srcnote location for the call is used to set `.columnNumber` and such when users do `Error()` to create an error, and for stack traces. By adding a new srcnote, I'm moving the column value from the `E` to just after the last `r`. Is that acceptable? We could special-case simple `ident()` calls to put the column at the start, which I'm for, but that still leaves edge cases where users have passed in constructors as arguments and such, like `function makeError(obj){ return (obj.cls || Error)(objmsg); }` or something, where the callee _isn't_ a name. The old behavior is probably equally confusing, so special-casing the simple-ident call is probably fine, but as a change that's technically user-visible (though implementation-defined), I wanted to ask.


diff --git a/js/src/frontend/BytecodeEmitter.cpp b/js/src/frontend/BytecodeEmitter.cpp
index fde7ee8..98e8ebe 100644
--- a/js/src/frontend/BytecodeEmitter.cpp
+++ b/js/src/frontend/BytecodeEmitter.cpp
@@ -9676,17 +9676,31 @@ BytecodeEmitter::emitCallOrNew(ParseNode* pn, ValueUsage valueUsage /* = ValueUs
         }
     }
 
+    ParseNode* coordNode = pn;
+    if (pn->getOp() == JSOP_CALL || pn->getOp() == JSOP_SPREADCALL) {
+        // The AST doesn't exactly represent the location of the `(` in a call,
+        // so we consider the end of the callee to be close enough.
+        if (!updateSourceCoordNotes(pn2->pn_pos.end))
+            return false;
+
+        // Clear the coordinate node since we already set a specific coordinate.
+        coordNode = nullptr;
+    }
+
     if (!spread) {
         if (pn->getOp() == JSOP_CALL && valueUsage == ValueUsage::IgnoreValue) {
-            if (!emitCall(JSOP_CALL_IGNORES_RV, argc, pn))
+            if (!emitCall(JSOP_CALL_IGNORES_RV, argc, coordNode))
                 return false;
             checkTypeSet(JSOP_CALL_IGNORES_RV);
         } else {
-            if (!emitCall(pn->getOp(), argc, pn))
+            if (!emitCall(pn->getOp(), argc, coordNode))
                 return false;
             checkTypeSet(pn->getOp());
         }
     } else {
+        if (coordNode && !updateSourceCoordNotes(coordNode->pn_pos.begin))
+            return false;
+
         if (!emit1(pn->getOp()))
             return false;
         checkTypeSet(pn->getOp());
status-firefox57: fix-optional → ---
(In reply to Logan Smyth from comment #2)
> 1. By using `pn2->pn_pos.end`, we're placing the breakpoint immediately
> after the callee. While that mostly works, it's not actually guaranteed to
> be the location of the `(` in the call, since there may be comments or
> whitespace separating the callee from the `(`. I can't see this information
> represented in the AST anywhere. Is there precedent for representing that?

We could change the AST to record the start of an argument list, but I like your next idea better:

> 2. It also seems like ideally we'd try to put the column at the start of
> properties instead of the `(`, for things like `obj.foo()` placing the
> column on the `f`, but the `name` doesn't appear to have location
> information. Is that worth adding?

The reason we don't have it is that our AST does not have a separate ParseNode for `foo`.
There's a node for `obj`, a node for `obj.foo`, and a node for `obj.foo()`.

To fix this, you must either

- add a node for the `foo` in `obj.foo`, changing PropertyAccess nodes from a direct subclass of ParseNode to a subclass of BinaryNode (and similarly changing everywhere ParseNodeKind::Dot is used);

- change the meaning of the location information on the existing `obj.foo` node to refer to the `foo` part, which is a bit wrong; OR

- find a way to make PropertyAccess nodes smuggle the extra offset in some unused field (ew).

The first option is best and the most work.
I should have mentioned one other case around my second point, which is that it applies for name-based property access (ParseNodeKind::Dot), but it's less clear for computed property access (ParseNodeKind::Elem), since you could have `obj["foo"]()`, and in that case it probably _does_ make sense to have the `(` be the column that we mark, since using the column of `[` would probably imply that the property hasn't been accessed yet.

Given that, even if we do want to go with changing the `ParseNode` to track the property location, which I'm a fan of, we'd still probably _also_ want the argument list location for the `(`.
One way to change the AST to include the location of the `(` is to make ParseNodeKind::Call nodes BinaryNodes rather than ListNodes. They'd have the callee expression on the left and ParseNodeKind::Arguments (a new kind of ListNode) on the right.

        x . y ( z )
        ^^^^^ ^^^^^
    callee       arguments

The position of the '(' would be `callNode->arguments()->pn_pos.begin`.
The spec grammar does sort of suggest this approach:

    CallExpression :
        CallExpression Arguments      # <--- two components! maybe it should be a binary node

Arguments can also be used for `new` and `super` calls; that's what the spec does, at least:

    SuperCall :
        `super` Arguments

    MemberExpression :
        `new` MemberExpression Arguments
Comment on attachment 8991713 [details]
Bug 1378808 - Add a new ParseNodeKind::Arguments node type for call argument lists.

https://reviewboard.mozilla.org/r/256658/#review263744

Looks great. Please update it with these few changes.

Every time I look at any code in the parser, I think "good gravy, what a mess!" and I feel a little ill... but then I remember this kind of change is actually relatively reasonable to make, and it's not untidiness per se that's the real nightmare in software.

::: js/src/frontend/BytecodeEmitter.cpp:9675
(Diff revision 1)
>          }
>      }
>  
> +    uint32_t argc = 0;
> +
> +    if (!emitArguments(pn_args, callop, spread, &argc))

Don't use an out-param for argc here; say `uint32_t argc = pn_args->pn_count;` here, and either repeat the `->pn_count` access inside the method (it'll be cheap) or pass `argc` in by value.

::: js/src/frontend/FoldConstants.cpp:1714
(Diff revision 1)
>  
>        case ParseNodeKind::Add:
>          return FoldAdd(cx, pnp, parser);
>  
>        case ParseNodeKind::Call:
> +      case ParseNodeKind::New:

Nice. I think the way ParseNodeKind::New was *not* included with the others may actually have been deliberate, but if so, it was outrageously subtle, so thanks!

::: js/src/frontend/NameFunctions.cpp:750
(Diff revision 1)
> +                return false;
> +            break;
> +
> +          // Handles the arguments for new/call/supercall, but does _not_ handle
> +          // the Arguments node used by tagged template literals, since that is
> +          // special-cased inside of resolveTaggedTemplate.

Nice comment. Thank you!

::: js/src/frontend/ParseNode.h:395
(Diff revision 1)
>   *                          pn_right: expr between [ and ]
> - * Call     list        pn_head: list of call, arg1, arg2, ... argN
> - *                          pn_count: 1 + N (where N is number of args)
> + * Call     binary      pn_left: callee expression on the left of the (
> + *                          pn_right: Arguments
> + * Arguments list       pn_head: list of arg1, arg2, ... argN
> + *                          pn_count: N (where N is number of args)
>   *                          call is a MEMBER expr naming a callable object

Please delete the line that says "call is a MEMBER expr..."

::: js/src/frontend/Parser.cpp:8712
(Diff revision 1)
>              bool matched;
>              if (!tokenStream.matchToken(&matched, TokenKind::Lp))
>                  return null();
> -            if (matched) {
> +
> -                bool isSpread = false;
> +            bool isSpread = false;
> -                if (!argumentList(yieldHandling, lhs, &isSpread))
> +            if (matched && !argumentList(yieldHandling, args, &isSpread))

The style rule in SM is to use two nested `if` statements if the purpose of one is to decide what to do, and the purpose of the other is error checking. So:

    if (matched) {
        if (!argumentList(...))
            return null();
    }
    
But I'm having you change this anyway (see below), so it'll be more like:

    Node args;
    if (matched) {
        args = argumentList(...);
        ...
    } else {
        args = handler.newArguments(pos());
        ...
    }

::: js/src/frontend/Parser.cpp:8793
(Diff revision 1)
>                  if (tt != TokenKind::Lp) {
>                      error(JSMSG_BAD_SUPER);
>                      return null();
>                  }
>  
> -                nextMember = handler.newSuperCall(lhs);
> +                Node args = handler.newArguments(pos());

Please put this `handler.newArguments()` call inside the `argumentList` method instead, and have it return a `Node`.

::: js/src/frontend/Parser.cpp:8898
(Diff revision 1)
> +
> +                    nextMember = handler.newTaggedTemplate(lhs, args);
> +                    if (!nextMember)
>                          return null();
> +
> +                    handler.setBeginPosition(nextMember, lhs);

This `handler.setBeginPosition()` call isn't necessary anymore, is it? `newTaggedTemplate` should take the begin position of `lhs`.
Attachment #8991713 - Flags: review?(jorendorff) → review+
Comment on attachment 8991714 [details]
Bug 1378808 - Add a new ParseNodeKind::PropertyName to hold location information about property access name.

https://reviewboard.mozilla.org/r/256660/#review264544

Looks good, please upload a new version with the issues fixed and set r?me again.

(You should actually be able to find these "issues" in the ReviewBoard interface somewhere and check them off. At least, I think everyone has enough permissions to do that. Let me know.)

::: js/src/frontend/BinSource.yaml:916
(Diff revision 2)
> +        property:
> +            block:
> +                before: |
> +                    namePos = tokenizer_->pos();
>      build: |
> -        BINJS_TRY_DECL(result, factory_.newPropertyAccess(object, property->asPropertyName(), start));
> +        auto name = factory_.newPropertyName(property->asPropertyName(), namePos);

Hmm. I think this needs to be BINJS_TRY_DECL too. I also don't know if it makes sense to grab a special position in this way; I'm going to flag Yoric for further review on this patch.

::: js/src/frontend/BytecodeEmitter.cpp:4314
(Diff revision 2)
>  
> -    ParseNode* pn2 = pn->pn_expr;
> +    ParseNode* pn2 = pn->pn_left;
>  
>      /*
>       * If the object operand is also a dotted property reference, reverse the
>       * list linked via pn_expr temporarily so we can iterate over it from the

Also change pn_expr here.

I had forgotten about this hack. :-|

::: js/src/frontend/BytecodeEmitter.cpp:4322
(Diff revision 2)
>      if (pn2->isKind(ParseNodeKind::Dot) && !pn2->as<PropertyAccess>().isSuper()) {
>          ParseNode* pndot = pn2;
>          ParseNode* pnup = nullptr;
>          ParseNode* pndown;
>          for (;;) {
>              /* Reverse pndot->pn_expr to point up, not down. */

and in this comment.

::: js/src/frontend/BytecodeEmitter.cpp:4421
(Diff revision 2)
>          if (!emitPropLHS(pn->pn_kid))               // OBJ
>              return false;
>          if (!emit1(JSOP_DUP))                       // OBJ OBJ
>              return false;
>      }
> -    if (!emitAtomOp(pn->pn_kid, isSuper? JSOP_GETPROP_SUPER : JSOP_GETPROP)) // OBJ V
> +    if (!emitAtomOp(pn->pn_kid->pn_right, isSuper? JSOP_GETPROP_SUPER : JSOP_GETPROP)) // OBJ V

While you're here, insert a space before that `?` please.

::: js/src/frontend/ParseNode.h:58
(Diff revision 2)
>      F(Neg) \
>      F(PreIncrement) \
>      F(PostIncrement) \
>      F(PreDecrement) \
>      F(PostDecrement) \
> +    F(Property) \

Please call it PropertyName instead, unless there is actual confusion between the ParseNodeKind and the type js::PropertyName (shouldn't be).

::: js/src/frontend/ParseNode.h:580
(Diff revision 2)
>                  JSAtom*      atom;      /* lexical name or label atom */
>                  ObjectBox*   objbox;    /* regexp object */
>                  FunctionBox* funbox;    /* function object */
>              };
>              ParseNode*  expr;           /* module or function body, var
> -                                           initializer, argument default, or
> +                                           initializer, argument default */

insert "or" before "argument default"

::: js/src/frontend/ParseNode.h:1188
(Diff revision 2)
>          MOZ_ASSERT_IF(match, node.isOp(JSOP_REGEXP));
>          return match;
>      }
>  };
>  
>  class PropertyAccess : public ParseNode

`: public BinaryNode` now

::: js/src/frontend/ParseNode.h:1196
(Diff revision 2)
> -    PropertyAccess(ParseNode* lhs, PropertyName* name, uint32_t begin, uint32_t end)
> -      : ParseNode(ParseNodeKind::Dot, JSOP_NOP, PN_NAME, TokenPos(begin, end))
> +    PropertyAccess(ParseNode* lhs, ParseNode* name, uint32_t begin, uint32_t end)
> +      : ParseNode(ParseNodeKind::Dot, JSOP_NOP, PN_BINARY, TokenPos(begin, end))
>      {
>          MOZ_ASSERT(lhs != nullptr);
>          MOZ_ASSERT(name != nullptr);
> -        pn_u.name.expr = lhs;
> +        pn_u.binary.left = lhs;

Please write `pn_left` rather than `pn_u.binary.left` throughout, and the same for `right`. It's the convention followed throughout the rest of the frontend, it's odd we used to break with it here...

::: js/src/frontend/ParseNode.h:1202
(Diff revision 2)
> -        pn_u.name.atom = name;
> +        pn_u.binary.right = name;
>      }
>  
>      static bool test(const ParseNode& node) {
>          bool match = node.isKind(ParseNodeKind::Dot);
> -        MOZ_ASSERT_IF(match, node.isArity(PN_NAME));
> +        MOZ_ASSERT_IF(match, node.isArity(PN_BINARY));

You can also `MOZ_ASSERT_IF(match,  pn_right->isKind(ParseNodeKind::PropertyName))`, right?

::: js/src/frontend/ParseNode.cpp:291
(Diff revision 2)
>  }
>  
>  void
>  NameNode::dump(GenericPrinter& out, int indent)
>  {
> -    if (isKind(ParseNodeKind::Name) || isKind(ParseNodeKind::Dot)) {
> +    if (isKind(ParseNodeKind::Name) || isKind(ParseNodeKind::Property)) {

I think this is wrong.

The code is DEBUG-only and really only for debugging use. Still, let's not break it. You can check it using the `parse()` shell builtin function. Right now the output is:

    js> parse("x.y.z")
    (StatementList [(ExpressionStatement (.z (.y x)))])

Changing to `(. (. x y) z)` would be fine -- anything within reason, really, but it seems like as written this is going to be weird.
Attachment #8991714 - Flags: review?(jorendorff) → review+
Comment on attachment 8991715 [details]
Bug 1378808 - Use ::Arguments or ::PropertyName location for method call column offsets.

https://reviewboard.mozilla.org/r/256662/#review264566

::: js/src/frontend/BytecodeEmitter.cpp:9710
(Diff revision 2)
> +            // Note: Because of the constant folding logic in FoldElement, this
> +            // case also applies for constant string properties.
> +            //
> +            // obj['aprop']() // expression
> +            //     ^          // column coord
> +            coordNode = pn_callee->pn_right;

I think we should only do this if pn_callee->pn_left is *not* an identifier, `this`, or `super`. That way, in the most common cases, users will never notice anything. Some of the existing tests won't change.

But I'll happily defer to whatever you and Jason think is the best user experience here.

::: js/src/frontend/BytecodeEmitter.cpp:9733
(Diff revision 2)
> -            if (!emitCall(pn->getOp(), argc, pn))
> +            if (!emitCall(pn->getOp(), argc, coordNode))
>                  return false;
>              checkTypeSet(pn->getOp());
>          }
>      } else {
> +        if (coordNode && !updateSourceCoordNotes(coordNode->pn_pos.begin))

Nested `if` statements, please.
Attachment #8991715 - Flags: review?(jorendorff) → review+
As with the others, please check off the issues, post a new patch r?me, and I'll r+ and get them landed as soon as I can.
(In reply to Jason Orendorff [:jorendorff] from comment #15)
> I think we should only do this if pn_callee->pn_left is *not* an identifier,
> `this`, or `super`.

It's tempting to keep going. In `this.motor.forward(30);` I'd be OK with putting the whole call expression at the column of either `this` or `forward`.
Comment on attachment 8991714 [details]
Bug 1378808 - Add a new ParseNodeKind::PropertyName to hold location information about property access name.

https://reviewboard.mozilla.org/r/256660/#review264544

> I think this is wrong.
> 
> The code is DEBUG-only and really only for debugging use. Still, let's not break it. You can check it using the `parse()` shell builtin function. Right now the output is:
> 
>     js> parse("x.y.z")
>     (StatementList [(ExpressionStatement (.z (.y x)))])
> 
> Changing to `(. (. x y) z)` would be fine -- anything within reason, really, but it seems like as written this is going to be weird.

Good catch, totally broken.
Logan, I found these new patches (looking specifically at the r2-to-r3 interdiffs) basically impossible to review, because of the mix of code motion with behavior changes. :(

I'm sure this sounds insane, especially since I already reviewed the other changes, and the new changes are obviously a great great thing. Ping me on slack or IRC and we can figure out how to do this.
Flags: needinfo?(loganfsmyth)
Comment on attachment 8991713 [details]
Bug 1378808 - Add a new ParseNodeKind::Arguments node type for call argument lists.

https://reviewboard.mozilla.org/r/256658/#review265772
Comment on attachment 8991714 [details]
Bug 1378808 - Add a new ParseNodeKind::PropertyName to hold location information about property access name.

https://reviewboard.mozilla.org/r/256660/#review265794

::: js/src/frontend/ParseNode.cpp:222
(Diff revisions 2 - 4)
>  BinaryNode::dump(GenericPrinter& out, int indent)
>  {
> +    if (isKind(ParseNodeKind::Dot)) {
> +        out.put("(.");
> +
> +        DumpParseTree(pn_right, out, indent + 2);

Cool, thanks!
Comment on attachment 8991715 [details]
Bug 1378808 - Use ::Arguments or ::PropertyName location for method call column offsets.

https://reviewboard.mozilla.org/r/256662/#review265796
Attachment #8991714 - Flags: review?(dteller)
Setting patch 2 r?Yoric, for further review of the changes to BinSource.yaml.

(I think it might be slightly off--although before long, this code will stop caring about ParseNodes anyway.)
Comment on attachment 8991714 [details]
Bug 1378808 - Add a new ParseNodeKind::PropertyName to hold location information about property access name.

https://reviewboard.mozilla.org/r/256660/#review266344

::: js/src/frontend/ParseNode.h:58
(Diff revision 4)
>      F(Neg) \
>      F(PreIncrement) \
>      F(PostIncrement) \
>      F(PreDecrement) \
>      F(PostDecrement) \
> +    F(PropertyName) \

Any chance you could take the opportunity to document what it means? Either here or in the long list before ParseArity?

::: js/src/frontend/ParseNode.h:1190
(Diff revision 4)
>  };
>  
> -class PropertyAccess : public ParseNode
> +class PropertyAccess : public BinaryNode
>  {
>    public:
> -    PropertyAccess(ParseNode* lhs, PropertyName* name, uint32_t begin, uint32_t end)
> +    PropertyAccess(ParseNode* lhs, ParseNode* name, uint32_t begin, uint32_t end)

Could you document what `name` needs to be?
Attachment #8991714 - Flags: review?(dteller) → review+
Pushed by jlaster@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b14186c3f895
Add a new ParseNodeKind::Arguments node type for call argument lists. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/e761b8eef0aa
Add a new ParseNodeKind::PropertyName to hold location information about property access name. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/8658a25ee96b
Use ::Arguments or ::PropertyName location for method call column offsets. r=jorendorff
Comment on attachment 8991713 [details]
Bug 1378808 - Add a new ParseNodeKind::Arguments node type for call argument lists.

https://reviewboard.mozilla.org/r/256656/#review267110

::: js/src/frontend/BinSource-auto.cpp:3744
(Diff revisions 5 - 6)
>  
>      BINJS_MOZ_TRY_DECL(object, parseExpressionOrSuper());
>  
>      BINJS_MOZ_TRY_DECL(expression, parseExpression());
>  
> -    BINJS_TRY_DECL(result, factory_.newPropertyByValue(object, expression, start));
> +    BINJS_TRY_DECL(result, factory_.newPropertyByValue(object, expression, tokenizer_->offset()));

As far as I can tell, this was an existing bug. newPropertyByValue expects the end location, but was being given the start location. I guess until now that wasn't an issue in the tests, but my change caused it to trigger an assertion about `begin > end`.

::: js/src/frontend/BinSource-auto.cpp:6206
(Diff revisions 5 - 6)
> -        namePos = tokenizer_->pos();
> +        nameStart = tokenizer_->offset();
>          MOZ_TRY_VAR(property, tokenizer_->readAtom());
>  
>      }
>  
> -    BINJS_TRY_DECL(name, factory_.newPropertyName(property->asPropertyName(), namePos));
> +    BINJS_TRY_DECL(name, factory_.newPropertyName(property->asPropertyName(), tokenizer_->pos(nameStart)));

Similar to above, I'd carried over the existing logic for passing the start even though the expected value was the end originally. Now this will actually pass a full start/end range for the name token.

::: js/src/frontend/BinSource.yaml:197
(Diff revisions 5 - 6)
>  
>              #endif // frontend_BinToken_h
>  
>  Arguments:
>      init:
> -        BINJS_TRY_DECL(result, factory_.newList(ParseNodeKind::ParamsBody, tokenizer_->pos(start)));
> +        BINJS_TRY_DECL(result, factory_.newList(ParseNodeKind::Arguments, tokenizer_->pos(start)));

Looks like this was an existing bug, but before the kind was always reset to `CallExpression` or `NewExpression` before use.

::: js/src/jsapi-tests/testSavedStacks.cpp:255
(Diff revision 6)
>      // Column
>      uint32_t column = 123;
>      result = JS::GetSavedFrameColumn(cx, selfHostedFrame, &column,
>                                       JS::SavedFrameSelfHosted::Exclude);
>      CHECK(result == JS::SavedFrameResult::Ok);
> -    CHECK_EQUAL(column, 5U);
> +    CHECK_EQUAL(column, 9U);

This is an expected change because of the way column offsets are different now.
Comment on attachment 8991715 [details]
Bug 1378808 - Use ::Arguments or ::PropertyName location for method call column offsets.

https://reviewboard.mozilla.org/r/256662/#review268276
Flags: needinfo?(jlaster)
After rebasing, there was one crashing test I didn't get around to investigating yesterday. It turns out to be pretty trivial, a missing OOM check which I've added. Re-running tests locally, then pushing to try...
Flags: needinfo?(loganfsmyth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e732697778c260c413047531e372a24286e0b667
Bug 1378808 - Add a new ParseNodeKind::Arguments node type for call argument lists. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/6dd9c641346afe439fb10c028becf0a7b1f0aedc
Bug 1378808 - Add a new ParseNodeKind::PropertyName to hold location information about property access name. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c6a609463ab5d71e475354cab8c3ff323d0571d
Bug 1378808 - Use ::Arguments or ::PropertyName location for method call column offsets. r=jorendorff
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
To clarify part 4:

The line with the CSP violation is:

    document.querySelector('#block6').contentDocument.body.appendChild(d);
    ^                                                      ^
    old columnNumber                                       new columnNumber

Previously, we reported an error at column 4, as the test requires. But I guess it's the .appendChild() method call that triggers the CSP violation. Now we report the error at column 59, the `a` of `appendChild`.
Flags: needinfo?(orendorf)
Flags: needinfo?(loganfsmyth)
Comment on attachment 8998987 [details] [diff] [review]
Part 4: Adjust web-platform-test to allow more precise error location

Review of attachment 8998987 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, r=me
Attachment #8998987 - Flags: review?(ckerschb) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb38cfb1031dcd9ca730d7aeac46c8dacf24c0e7
Bug 1378808 - Add a new ParseNodeKind::Arguments node type for call argument lists. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/b789f764a1ae22d4b6f09658a9e0d79791b50125
Bug 1378808 - Add a new ParseNodeKind::PropertyName to hold location information about property access name. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/50338591c8f31326a22921c10cc470849aa4e430
Bug 1378808 - Use ::Arguments or ::PropertyName location for method call column offsets. r=jorendorff, r=ckerschb
https://hg.mozilla.org/mozilla-central/rev/fb38cfb1031d
https://hg.mozilla.org/mozilla-central/rev/b789f764a1ae
https://hg.mozilla.org/mozilla-central/rev/50338591c8f3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12463 for changes under testing/web-platform/tests
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: