Fix issues with duplicate key logic by rbuckton · Pull Request #79 · tc39/proposal-decorators

rbuckton · 2018-03-21T21:12:49Z

Fixes #78

nicolo-ribaudo · 2018-03-25T17:29:40Z

spec.html

      <emu-alg>
        1. Let _extras_ be a new empty List.
        1. Let _finishers_ be a new empty List.
+        1. Let _placementKeys_ be *undefined*.


I think that it should be ~empty~, since *undefined* is the JavaScript value. Also, maybe it can just be used

1. If _element_.[[Placement]] is `"own"`, let _placementKeys_ to _keys_.[[OwnKeys]]. ...

without declaring _placementKeys_ first.

(example: https://tc39.github.io/ecma262/#sec-toprimitive)

nicolo-ribaudo · 2018-03-25T17:33:08Z

spec.html

+          1. Else if _element_.[[Placement]] is `"static"`, then
+            1. set _placementKeys_ to _keys_.[[StaticKeys]].
+          1. Else,
+            1. set _placementKeys_ to _keys_.[[PrototypeKeys]].


Since this if/else if/else steps are used different times, maybe we can extract them into a different "function"?
e.g.

1. Let _placementKeys_ be GetPlacementKeys(_keys_, _element_.[[Placement]])

I didn't want to pollute the spec with an additional tangential function.

Personally I’d prefer more abstract operations if it can clean up the algorithms, but generally it’s best to only do so when things are very complex or needed in more than one place.

tc39/proposal-decorators#79

littledan · 2018-04-03T10:54:39Z

spec.html

-          1. Append _element_.[[Key]] to _keys_.
+          1. Perform ? AddElementPlacement(_element_, _placements_).
        1. For each _element_ in _elements_, do
          1. Let _elementFinishersExtras_ be ? DecorateElement(_element_, _keys_).


Should you pass placements rather than keys here?

littledan · 2018-04-03T10:55:46Z

spec.html

-          1. Assert: _element_.[[Key]] is an element of _keys_.
-          1. Remove _element_.[[Key]] from _keys_.
+        1. For each _decorator_ in _element_.[[Decorators]], in reverse list order do
+          1. Perform RemoveElementPlacement(_element_, _placements_).


This process of removing and re-adding the elements will change the enumeration order, e.g. extras may precede the element. Is this intentional?

This doesn't change order at all. placements are just bags of property names.

Right, never mind.

rbuckton · 2018-04-12T18:25:30Z

I made an additional change to not throw a TypeError when initially creating placements, since we don't deduplicate fields in CoalesceClassElements.

rbuckton · 2018-04-12T18:30:03Z

@littledan I've addressed PR feedback and fixed merge conflicts with master. Can you take another look?

rbuckton · 2018-04-12T18:33:27Z

spec.html

+        1. Let _placements_ be the Record { [[StaticKeys]]: « », [[PrototypeKeys]]: « », [[OwnKeys]]: « » }.
        1. For each _element_ in _elements_, do
-          1. Append _element_.[[Key]] to _keys_.
+          1. Perform ? AddElementPlacement(_element_, _placements_, *true*).


Initially I assumed that elements was already deduplicated from CoalesceClassElements, so it would be fine to reuse AddElementPlacement as it wouldn't introduce an error for user-supplied declarations. However, upon rereading that algorithm it seems that we don't coalesce field initializers, so there exists the possibility of user-written duplicates. Since this mechanism is only to prevent decorators from adding a duplicate definition, I've modified AddElementPlacement to accept an optional silent argument to suppress errors when we initially create placements.

Good call, makes sense to me.

littledan

Thanks for thinking this all through.

littledan · 2018-04-14T01:20:39Z

spec.html

-          1. Assert: _element_.[[Key]] is an element of _keys_.
-          1. Remove _element_.[[Key]] from _keys_.
+        1. For each _decorator_ in _element_.[[Decorators]], in reverse list order do
+          1. Perform RemoveElementPlacement(_element_, _placements_).


Right, never mind.

littledan · 2018-04-15T22:24:47Z

spec.html

+        1. Let _placements_ be the Record { [[StaticKeys]]: « », [[PrototypeKeys]]: « », [[OwnKeys]]: « » }.
        1. For each _element_ in _elements_, do
-          1. Append _element_.[[Key]] to _keys_.
+          1. Perform ? AddElementPlacement(_element_, _placements_, *true*).


Good call, makes sense to me.

littledan · 2018-04-15T22:25:15Z

Could you rebase the patch?

rbuckton · 2018-04-17T19:49:33Z

@littledan rebased

littledan · 2018-04-17T21:19:43Z

Thanks!

tc39/proposal-decorators#79

nicolo-ribaudo reviewed Mar 25, 2018

View reviewed changes

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Mar 25, 2018

Duplicated keys with different placement

ed0977e

tc39/proposal-decorators#79

rbuckton force-pushed the fix-duplicate-key-logic branch from ac4a9ad to 86d108b Compare March 27, 2018 22:00

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Mar 29, 2018

Duplicated keys with different placement

a035412

tc39/proposal-decorators#79

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Apr 3, 2018

Duplicated keys with different placement

74df1f6

tc39/proposal-decorators#79

littledan requested changes Apr 3, 2018

View reviewed changes

littledan added the stage 3 blocker label Apr 3, 2018

rbuckton commented Apr 12, 2018

View reviewed changes

littledan approved these changes Apr 15, 2018

View reviewed changes

rbuckton added 3 commits April 17, 2018 12:48

Fix issues with duplicate key logic

555b3d2

Abstract operations for validating placement

4f9eef1

PR Feedback and avoid error when initially creating 'placements'

5a4f66d

rbuckton force-pushed the fix-duplicate-key-logic branch from 02ca37d to 5a4f66d Compare April 17, 2018 19:48

littledan merged commit 694bb37 into tc39:master Apr 17, 2018

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jun 6, 2018

[spec update] Fix duplicated keys logic

5bd0717

tc39/proposal-decorators#79

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jun 8, 2018

[spec update] Fix duplicated keys logic

09e5e95

tc39/proposal-decorators#79

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jul 1, 2018

[spec update] Fix duplicated keys logic

11c6411

tc39/proposal-decorators#79

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jul 3, 2018

[spec update] Fix duplicated keys logic

d9e5e2c

tc39/proposal-decorators#79

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jul 10, 2018

[spec update] Fix duplicated keys logic

8541e75

tc39/proposal-decorators#79

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jul 28, 2018

[spec update] Fix duplicated keys logic

57f91fa

tc39/proposal-decorators#79

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Aug 17, 2018

[spec update] Fix duplicated keys logic

e802a6c

tc39/proposal-decorators#79

Conversation

rbuckton commented Mar 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljharb Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbuckton commented Apr 12, 2018

Uh oh!

rbuckton commented Apr 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

littledan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

littledan commented Apr 15, 2018

Uh oh!

rbuckton commented Apr 17, 2018

Uh oh!

littledan commented Apr 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ljharb Mar 27, 2018 •

edited

Loading