Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2255353002:
[parser] Allow duplicate __proto__ keys in patterns (Closed)
Description[parser] Allow duplicate __proto__ keys in patterns
This patch subsumes CoverInitializedNameProduction to create an ObjectLiteralProduction which is now used to report the duplicate proto error as well.
This patch also changes ObjectLiteralChecker::CheckProperty
to record an ObjectLiteralProduction error instead of
bailing out immediately. Once we realize that we're in a
pattern, we rewind the error, otherwise we report the
error.
BUG=v8:5121
Committed: https://crrev.com/fc52e323611a3f2e5812611a2a4915e6760c65e5
Cr-Commit-Position: refs/heads/master@{#38764}
Patch Set 1 #Patch Set 2 : rebase with master #Patch Set 3 : fix rebase issues #
Total comments: 8
Patch Set 4 : review comments #
Total comments: 5
Patch Set 5 : create objectliteral production #
Total comments: 3
Patch Set 6 : remove some test #
Messages
Total messages: 42 (29 generated)
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/11185) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/11180) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/22072)
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [parser] Allow duplicate __proto__ keys in patterns This patch changes ObjectLiteralChecker::CheckProperty to record the Exprssion error instead of bailing out immediately. Once we realize that we're in a pattern, we rewind the Expression error, otherwise we bail out. BUG=v8:5121 ========== to ========== [parser] Allow duplicate __proto__ keys in patterns This patch changes ObjectLiteralChecker::CheckProperty to record the Exprssion error instead of bailing out immediately. Once we realize that we're in a pattern, we rewind the Expression error, otherwise we report the error. BUG=v8:5121 ==========
gsathya@chromium.org changed reviewers: + adamk@chromium.org, littledan@chromium.org
gsathya@chromium.org changed reviewers: + adamk@chromium.org, littledan@chromium.org
Main confusion is why the need to add ForgiveExpressionError... https://codereview.chromium.org/2255353002/diff/40001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2255353002/diff/40001/src/parsing/parser-base... src/parsing/parser-base.h:2418: classifier->ForgiveExpressionError(); Is this necessary? What fails if you don't "forgive"? https://codereview.chromium.org/2255353002/diff/40001/src/parsing/parser-base... src/parsing/parser-base.h:2449: if (!classifier->is_valid_expression()) Was this necessary? I wouldn't think you'd need to add this. https://codereview.chromium.org/2255353002/diff/40001/src/parsing/parser-base... src/parsing/parser-base.h:3690: ExpressionClassifier* classifier, bool* ok) { Hmm, it's a little strange that this takes "ok" now that it can never fail, but I guess that's needed because of the class case? https://codereview.chromium.org/2255353002/diff/40001/test/test262/test262.st... File test/test262/test262.status (left): https://codereview.chromium.org/2255353002/diff/40001/test/test262/test262.st... test/test262/test262.status:356: 'language/expressions/assignment/destructuring/obj-prop-__proto__dup': [FAIL], Please add a test-parsing.cc test as well to make sure we're covering both the parser and preparser (this test will likely only exercise the parser).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [parser] Allow duplicate __proto__ keys in patterns This patch changes ObjectLiteralChecker::CheckProperty to record the Exprssion error instead of bailing out immediately. Once we realize that we're in a pattern, we rewind the Expression error, otherwise we report the error. BUG=v8:5121 ========== to ========== [parser] Allow duplicate __proto__ keys in patterns This patch adds an AnnexBDuplicateProtoProduction to the list of errors in the expression classifier, along with methods to record and forgive this error. This patch also changes ObjectLiteralChecker::CheckProperty to record an AnnexBDuplicateProto error instead of bailing out immediately. Once we realize that we're in a pattern, we rewind the error, otherwise we report the error. BUG=v8:5121 ==========
PTAL https://codereview.chromium.org/2255353002/diff/40001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2255353002/diff/40001/src/parsing/parser-base... src/parsing/parser-base.h:2418: classifier->ForgiveExpressionError(); On 2016/08/18 20:35:22, adamk wrote: > Is this necessary? What fails if you don't "forgive"? As discussed offline -- leaving this here. https://codereview.chromium.org/2255353002/diff/40001/src/parsing/parser-base... src/parsing/parser-base.h:2449: if (!classifier->is_valid_expression()) On 2016/08/18 20:35:22, adamk wrote: > Was this necessary? I wouldn't think you'd need to add this. Removed. https://codereview.chromium.org/2255353002/diff/40001/src/parsing/parser-base... src/parsing/parser-base.h:3690: ExpressionClassifier* classifier, bool* ok) { On 2016/08/18 20:35:22, adamk wrote: > Hmm, it's a little strange that this takes "ok" now that it can never fail, but > I guess that's needed because of the class case? Yeah :/ It's the same with classifier being added to ClassLiteralChecker::CheckProperty. https://codereview.chromium.org/2255353002/diff/40001/test/test262/test262.st... File test/test262/test262.status (left): https://codereview.chromium.org/2255353002/diff/40001/test/test262/test262.st... test/test262/test262.status:356: 'language/expressions/assignment/destructuring/obj-prop-__proto__dup': [FAIL], On 2016/08/18 20:35:22, adamk wrote: > Please add a test-parsing.cc test as well to make sure we're covering both the > parser and preparser (this test will likely only exercise the parser). Added. Let me know if there if there are any that are redundant (or others that I may have missed).
https://codereview.chromium.org/2255353002/diff/60001/src/parsing/expression-... File src/parsing/expression-classifier.h (right): https://codereview.chromium.org/2255353002/diff/60001/src/parsing/expression-... src/parsing/expression-classifier.h:29: T(AnnexBDuplicateProtoProduction, 12) Let's just call this DuplicateProtoProduction. https://codereview.chromium.org/2255353002/diff/60001/src/parsing/parser-base.h File src/parsing/parser-base.h (left): https://codereview.chromium.org/2255353002/diff/60001/src/parsing/parser-base... src/parsing/parser-base.h:917: if (a.beg_pos < 0 || (b.beg_pos >= 0 && a.beg_pos > b.beg_pos)) { Ugh, I forgot about this logic, it's trying to determine which error was first. What if we combined DuplicateProtoProduction with CoverInitializedNameProduction (not sure what the right name is...)? Or maybe there should only be a single slot for ExpressionProduction? Would be interesting to see what tests fail if we combine CoverInitializedName with Expression in the existing code.
caitp@igalia.com changed reviewers: + caitp@igalia.com
https://codereview.chromium.org/2255353002/diff/60001/src/parsing/parser-base.h File src/parsing/parser-base.h (left): https://codereview.chromium.org/2255353002/diff/60001/src/parsing/parser-base... src/parsing/parser-base.h:917: if (a.beg_pos < 0 || (b.beg_pos >= 0 && a.beg_pos > b.beg_pos)) { On 2016/08/19 20:17:08, adamk wrote: > Ugh, I forgot about this logic, it's trying to determine which error was first. > > What if we combined DuplicateProtoProduction with CoverInitializedNameProduction > (not sure what the right name is...)? > > Or maybe there should only be a single slot for ExpressionProduction? Would be > interesting to see what tests fail if we combine CoverInitializedName with > Expression in the existing code. IIRC, the reason for this is related to how arrow functions get validated --- there was some requirement that `({ a = 10 }) =>` be validated as an Expression before the `=>` gets parsed and processed (this would be in ParseExpression). That's the only reason for the difference, though.
Description was changed from ========== [parser] Allow duplicate __proto__ keys in patterns This patch adds an AnnexBDuplicateProtoProduction to the list of errors in the expression classifier, along with methods to record and forgive this error. This patch also changes ObjectLiteralChecker::CheckProperty to record an AnnexBDuplicateProto error instead of bailing out immediately. Once we realize that we're in a pattern, we rewind the error, otherwise we report the error. BUG=v8:5121 ========== to ========== [parser] Allow duplicate __proto__ keys in patterns This patch subsumes CoverInitializedNameProduction to create an ObjectLiteralProduction which is now used to report the duplicate proto error as well. This patch also changes ObjectLiteralChecker::CheckProperty to record an ObjectLiteralProduction error instead of bailing out immediately. Once we realize that we're in a pattern, we rewind the error, otherwise we report the error. BUG=v8:5121 ==========
gsathya@chromium.org changed reviewers: - caitp@igalia.com
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
gsathya@chromium.org changed reviewers: + caitp@igalia.com
PTAL https://codereview.chromium.org/2255353002/diff/60001/src/parsing/expression-... File src/parsing/expression-classifier.h (right): https://codereview.chromium.org/2255353002/diff/60001/src/parsing/expression-... src/parsing/expression-classifier.h:29: T(AnnexBDuplicateProtoProduction, 12) On 2016/08/19 20:17:07, adamk wrote: > Let's just call this DuplicateProtoProduction. Changed to ObjectLiteralProduction as discussed offline https://codereview.chromium.org/2255353002/diff/60001/src/parsing/parser-base.h File src/parsing/parser-base.h (left): https://codereview.chromium.org/2255353002/diff/60001/src/parsing/parser-base... src/parsing/parser-base.h:917: if (a.beg_pos < 0 || (b.beg_pos >= 0 && a.beg_pos > b.beg_pos)) { On 2016/08/19 20:17:08, adamk wrote: > Ugh, I forgot about this logic, it's trying to determine which error was first. > > What if we combined DuplicateProtoProduction with CoverInitializedNameProduction > (not sure what the right name is...)? > > Or maybe there should only be a single slot for ExpressionProduction? Would be > interesting to see what tests fail if we combine CoverInitializedName with > Expression in the existing code. I combined CoverInitializedName with this for now.
lgtm % tests https://codereview.chromium.org/2255353002/diff/80001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2255353002/diff/80001/test/cctest/test-parsin... test/cctest/test-parsing.cc:6790: "{{ __proto__: x, __proto__: y}}", Why the double braces? Isn't that already an error for other reasons? https://codereview.chromium.org/2255353002/diff/80001/test/cctest/test-parsin... test/cctest/test-parsing.cc:6791: "({ __proto__: x, __proto__: y})", Same here, I think the parens are already an error. https://codereview.chromium.org/2255353002/diff/80001/test/cctest/test-parsin... test/cctest/test-parsing.cc:6879: "var {{ __proto__: x, __proto__: y}}", And here and below.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/19 23:27:35, adamk wrote: > lgtm % tests > > https://codereview.chromium.org/2255353002/diff/80001/test/cctest/test-parsin... > File test/cctest/test-parsing.cc (right): > > https://codereview.chromium.org/2255353002/diff/80001/test/cctest/test-parsin... > test/cctest/test-parsing.cc:6790: "{{ __proto__: x, __proto__: y}}", > Why the double braces? Isn't that already an error for other reasons? > > https://codereview.chromium.org/2255353002/diff/80001/test/cctest/test-parsin... > test/cctest/test-parsing.cc:6791: "({ __proto__: x, __proto__: y})", > Same here, I think the parens are already an error. > > https://codereview.chromium.org/2255353002/diff/80001/test/cctest/test-parsin... > test/cctest/test-parsing.cc:6879: "var {{ __proto__: x, __proto__: y}}", > And here and below. Removed. Blanking on why I added it.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gsathya@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [parser] Allow duplicate __proto__ keys in patterns This patch subsumes CoverInitializedNameProduction to create an ObjectLiteralProduction which is now used to report the duplicate proto error as well. This patch also changes ObjectLiteralChecker::CheckProperty to record an ObjectLiteralProduction error instead of bailing out immediately. Once we realize that we're in a pattern, we rewind the error, otherwise we report the error. BUG=v8:5121 ========== to ========== [parser] Allow duplicate __proto__ keys in patterns This patch subsumes CoverInitializedNameProduction to create an ObjectLiteralProduction which is now used to report the duplicate proto error as well. This patch also changes ObjectLiteralChecker::CheckProperty to record an ObjectLiteralProduction error instead of bailing out immediately. Once we realize that we're in a pattern, we rewind the error, otherwise we report the error. BUG=v8:5121 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [parser] Allow duplicate __proto__ keys in patterns This patch subsumes CoverInitializedNameProduction to create an ObjectLiteralProduction which is now used to report the duplicate proto error as well. This patch also changes ObjectLiteralChecker::CheckProperty to record an ObjectLiteralProduction error instead of bailing out immediately. Once we realize that we're in a pattern, we rewind the error, otherwise we report the error. BUG=v8:5121 ========== to ========== [parser] Allow duplicate __proto__ keys in patterns This patch subsumes CoverInitializedNameProduction to create an ObjectLiteralProduction which is now used to report the duplicate proto error as well. This patch also changes ObjectLiteralChecker::CheckProperty to record an ObjectLiteralProduction error instead of bailing out immediately. Once we realize that we're in a pattern, we rewind the error, otherwise we report the error. BUG=v8:5121 Committed: https://crrev.com/fc52e323611a3f2e5812611a2a4915e6760c65e5 Cr-Commit-Position: refs/heads/master@{#38764} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/fc52e323611a3f2e5812611a2a4915e6760c65e5 Cr-Commit-Position: refs/heads/master@{#38764} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
