Bug 197603 - [JSC] Invalid AssignmentTargetType should be an early error.
Summary: [JSC] Invalid AssignmentTargetType should be an early error.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
: 190889 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-05-05 22:09 PDT by Ross Kirsling
Modified: 2019-05-16 13:09 PDT (History)
10 users (show)

See Also:


Attachments
Patch (96.17 KB, patch)
2019-05-05 22:57 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (3.15 MB, application/zip)
2019-05-06 00:03 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.71 MB, application/zip)
2019-05-06 00:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-highsierra (2.93 MB, application/zip)
2019-05-06 00:46 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews213 for win-future (13.49 MB, application/zip)
2019-05-06 00:56 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (12.46 MB, application/zip)
2019-05-06 00:57 PDT, Build Bot
no flags Details
Patch (105.54 KB, patch)
2019-05-06 11:02 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (106.05 KB, patch)
2019-05-06 11:32 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (108.60 KB, patch)
2019-05-06 21:03 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews213 for win-future (13.80 MB, application/zip)
2019-05-07 18:50 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2019-05-05 22:09:25 PDT
Expressions like 0++, ++0, 0 = 0, and 0 += 0 are all specified as early errors, while JSC handles them as late errors:
https://tc39.github.io/ecma262/#sec-update-expressions-static-semantics-early-errors
https://tc39.github.io/ecma262/#sec-assignment-operators-static-semantics-early-errors

In particular, they're all early ReferenceErrors, a notion that JSC doesn't currently have; however, as I noted in bug 177218 comment 2, I am in the midst of proposing that we drop this notion and let all early errors be SyntaxErrors:
https://github.com/tc39/ecma262/pull/1527

With the hope that the above PR will meet with consensus in one month's time, I'd like to go ahead and turn these into early SyntaxErrors.
Then if all goes well, we can simply do a Test262 import after the spec update.
Comment 1 Ross Kirsling 2019-05-05 22:09:44 PDT
*** Bug 190889 has been marked as a duplicate of this bug. ***
Comment 2 Ross Kirsling 2019-05-05 22:57:33 PDT Comment hidden (obsolete)
Comment 3 Ross Kirsling 2019-05-05 23:00:36 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2019-05-06 00:03:14 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2019-05-06 00:03:15 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2019-05-06 00:15:25 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2019-05-06 00:15:27 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2019-05-06 00:46:39 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2019-05-06 00:46:41 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2019-05-06 00:56:21 PDT Comment hidden (obsolete)
Comment 11 Build Bot 2019-05-06 00:56:24 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2019-05-06 00:57:10 PDT Comment hidden (obsolete)
Comment 13 Build Bot 2019-05-06 00:57:12 PDT Comment hidden (obsolete)
Comment 14 Ross Kirsling 2019-05-06 11:02:20 PDT Comment hidden (obsolete)
Comment 15 Ross Kirsling 2019-05-06 11:32:45 PDT Comment hidden (obsolete)
Comment 16 Ross Kirsling 2019-05-06 13:43:16 PDT
Comment on attachment 369147 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.cpp:3749
>          if (UNLIKELY(context.isMetaProperty(lhs)))
>              internalFailWithMessage(false, metaPropertyName(context, lhs), " can't be the left hand side of an assignment expression");
> +        semanticFailIfFalse(isSimpleAssignmentTarget(context, lhs), "Left side of assignment is not a reference");

Note:
All of these isSimpleAssignmentTarget checks are colocated with !isMetaProperty checks -- the former subsumes the latter, so it might be nice to merge them.
I've avoided touching them for now simply because I wanted to preserve error messages in the initial patch for review.
(Notice that the only time an error *message* changes here is when hitting a requiresLExpr check (e.g. ++--x) in loose mode, since those checks used to be strict-mode-only and have their own unique messaging.)
Comment 17 Ross Kirsling 2019-05-06 21:03:09 PDT
Created attachment 369224 [details]
Patch
Comment 18 Build Bot 2019-05-07 18:50:49 PDT Comment hidden (obsolete)
Comment 19 Build Bot 2019-05-07 18:50:53 PDT Comment hidden (obsolete)
Comment 20 Ross Kirsling 2019-05-09 18:24:22 PDT
Reminder that this is ready for review. :)
Comment 21 Ross Kirsling 2019-05-16 10:33:50 PDT
Ping?
Comment 22 Keith Miller 2019-05-16 12:11:19 PDT
Comment on attachment 369224 [details]
Patch

r=me. I'm not 100% sure we should do SyntaxError until we have agreement at tc-39 but since I think that's the right choice anyway I'm ok with it. Can you file a bugzilla to track the PR on tc-39 though?
Comment 23 Ross Kirsling 2019-05-16 12:39:05 PDT
(In reply to Keith Miller from comment #22)
> Comment on attachment 369224 [details]
> Patch
> 
> r=me. I'm not 100% sure we should do SyntaxError until we have agreement at
> tc-39 but since I think that's the right choice anyway I'm ok with it. Can
> you file a bugzilla to track the PR on tc-39 though?

Thanks Keith! I'm leaving bug 177218 open for that purpose.
Comment 24 WebKit Commit Bot 2019-05-16 13:08:28 PDT
Comment on attachment 369224 [details]
Patch

Clearing flags on attachment: 369224

Committed r245406: <https://trac.webkit.org/changeset/245406>
Comment 25 WebKit Commit Bot 2019-05-16 13:08:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Radar WebKit Bug Importer 2019-05-16 13:09:38 PDT
<rdar://problem/50864605>