Bug 217879 - test262: test/language/expressions/conditional/in-branch-1.js
Summary: test262: test/language/expressions/conditional/in-branch-1.js
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Cohen
Keywords: InRadar
Depends on:
Reported: 2020-10-17 14:15 PDT by Mark Cohen
Modified: 2020-10-19 14:15 PDT (History)
10 users (show)

See Also:

Patch (4.33 KB, patch)
2020-10-17 18:36 PDT, Mark Cohen
no flags Details | Formatted Diff | Diff
Patch (6.32 KB, patch)
2020-10-18 16:56 PDT, Mark Cohen
no flags Details | Formatted Diff | Diff
Patch (6.32 KB, patch)
2020-10-19 10:06 PDT, Mark Cohen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Cohen 2020-10-17 14:15:34 PDT
The in-branch-1 test262 test is failing, which checks that the `in` keyword is allowed in the LHS of a ternary (between `?` and `:`) even if that ternary is one of the expressions in a `for` statement.
Comment 1 Mark Cohen 2020-10-17 18:36:24 PDT
Created attachment 411692 [details]
Comment 2 Mark Cohen 2020-10-17 18:41:57 PDT
I ran test262 before and after and I can confirm that this patch only affects in-branch-1. I also ran run-javascriptcore-tests and got 0 failures.
Comment 3 Darin Adler 2020-10-18 13:32:00 PDT
Comment on attachment 411692 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=411692&action=review

WebKit project bug fixes require including a test with the patch.

Given this fixes a test, why is there no change to test expectations showing the newly passing test?

> Source/JavaScriptCore/parser/Parser.cpp:4032
> +    bool oldAllowsIn = m_allowsIn;
> +    m_allowsIn = true;
>      TreeExpression lhs = parseAssignmentExpression(context);
> +    m_allowsIn = oldAllowsIn;

Looks fine to me, but there are two other things we sometimes use in cases like this:

1) SetForScope, so we don’t need to do the restore explicitly, but requires a scope.
2) std::exchange so we can get and set something at the same time.
Comment 4 Alexey Shvayka 2020-10-18 14:04:30 PDT
Comment on attachment 411692 [details]

That's a great first patch, Mark!

r=me with 2 comments:

1. Let's AllowInOverride instead.
2. Please remove now passing test from JSTests/test262/expectations.yaml.
Comment 5 Mark Cohen 2020-10-18 16:56:58 PDT
Created attachment 411722 [details]
Comment 6 Mark Cohen 2020-10-18 16:58:10 PDT
thanks all for the comments! just uploaded an updated patch. let me know if there's anything else I should address.
Comment 7 Alexey Shvayka 2020-10-19 04:54:21 PDT
Comment on attachment 411722 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=411722&action=review

> Source/JavaScriptCore/parser/Parser.cpp:4029
> +    TreeExpression lhs;

The final nit: let's avoid calling default constructor by doing `TreeExpression lhs = 0`, which seems like a convention in Parser.cpp.
Please re-upload the patch with --no-review, since it was already reviewed, and with --request-commit so a WebKit committer would help you land it.
Comment 8 Mark Cohen 2020-10-19 10:06:31 PDT
Created attachment 411758 [details]
Comment 9 Mark Cohen 2020-10-19 10:06:58 PDT
done and done. thanks!
Comment 10 Alexey Shvayka 2020-10-19 12:22:02 PDT
Comment on attachment 411758 [details]

Congratulations on your first patch landed! Great job, Mark!
Comment 11 EWS 2020-10-19 12:22:41 PDT
Downloading keyring-7.3.1...
Installing keyring-7.3.1...
Installed keyring-7.3.1!
ChangeLog entry in JSTests/ChangeLog contains OOPS!.
Comment 12 Alexey Shvayka 2020-10-19 12:31:02 PDT
(In reply to EWS from comment #11)
> ChangeLog entry in JSTests/ChangeLog contains OOPS!.

Hmm, I guess reviewer's name is required to be filled manually.
Comment 13 Darin Adler 2020-10-19 12:31:45 PDT
Comment on attachment 411758 [details]

Lets try that again.
Comment 14 Darin Adler 2020-10-19 12:32:33 PDT
I set review+ to try again.

It might be the "wrong" reviewer name (mine), but should work.
Comment 15 EWS 2020-10-19 14:14:09 PDT
Committed r268688: <https://trac.webkit.org/changeset/268688>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411758 [details].
Comment 16 Radar WebKit Bug Importer 2020-10-19 14:15:19 PDT