Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[parser] Disallow duplicate and undeclared private names #10456

Merged
merged 12 commits into from Jan 10, 2020

Conversation

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Sep 17, 2019

Q                       A
Fixed Issues?
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

Note: the tests inside class-private-names-duplicated are automatically generated. They are (instance|static*field|method|get|set)^2

This PR reduces the Test262 whitelist from 194 entries to 18 🎉 We are now 99.97% compliant.

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Sep 17, 2019

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11701/

@JLHwung

This comment has been minimized.

Copy link
Contributor

JLHwung commented Sep 17, 2019

As long as we make it syntax error finally, do we need check duplicated private name in the transformer?

if (
privateNames.has(getName) ||
(privateNames.has(name) && !privateNames.has(setName))
) {
throw path.buildCodeFrameError("Duplicate private field");
}

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Sep 17, 2019

I'm ok with removing it since we don't check other early errors in the transform plugins, but:

  • we already report errors about duplicate variables also after parsing, in case the AST is modified. I think that these two kinds of errors are similar;
  • it will accept invalid code in case someone has @babel/helper-create-class-features-plugin >v7.6.1 and @babel/parser <v7.6.1 (not that it is a big deal)
@JLHwung

This comment has been minimized.

Copy link
Contributor

JLHwung commented Sep 17, 2019

we already report errors about duplicate variables also after parsing, in case the AST is modified

If we want to report errors on duplicated private names after parsing, ideally it should be done in babel-traverse instead of the plugin since we could not make any assumptions of plugin ordering.

@babel/helper-create-class-features-plugin >v7.6.1 and @babel/parser <v7.6.1

We could bump its peerDependency of @babel/core to 7.6.1 or let's target to v8 release.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Sep 17, 2019

If we want to report errors on duplicated private names after parsing, ideally it should be done in babel-traverse instead of the plugin since we could not make any assumptions of plugin ordering

Well, we can be sure that this plugin will be the last one run on private names, since then they are removed.

We could bump its peerDependency of @babel/core to 7.6.1 or let's target to v8 release.

This is a breaking change, since npm would suddently start to warn about valid dependencies.


I'm not going to die on this hill since I am just slightly in favor of the duplicated error 😛
@babel/babel @tim-mc Do you think that we can remove it from the transform plugin?

@JLHwung

This comment has been minimized.

Copy link
Contributor

JLHwung commented Sep 17, 2019

Well, we can be sure that this plugin will be the last one run on private names, since then they are removed.

What would happen if a plugin inserts private names after they are removed? Would it still be transpiled?

If it is a breaking change to bump peerDependencies, we could target to v8 anyway. Not a big deal.

@tim-mc

This comment has been minimized.

Copy link
Contributor

tim-mc commented Sep 17, 2019

It makes sense to move it to babel-traverse given some of the above reasons (plugin ordering, reporting the error after parsing). Also, if errors aren't typically reported in the transforms, it's probably best to stick to that convention rather than unnecessarily spreading that concern out to more areas of the codebase.

I also agree that v8 should be targeted for the change.

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:duplicate-private branch from 73b3231 to 13dcae6 Sep 18, 2019
@nicolo-ribaudo nicolo-ribaudo dismissed JLHwung’s stale review Sep 18, 2019

I noticed that private names are much more complex to handle since undeclared private names are an early error. I'm mostly rewriting this PR, but all the test changes and the logic to handle accessors will remain the same.

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:duplicate-private branch from 13dcae6 to 8e836dd Sep 18, 2019
@nicolo-ribaudo nicolo-ribaudo changed the title [parser] Disallow duplicate private names [parser] Disallow duplicate and undeclared private names Sep 18, 2019
language/expressions/object/method-definition/private-name-early-error-async-gen-inside-class.js(default)
language/expressions/object/method-definition/private-name-early-error-async-gen-inside-class.js(strict mode)
language/expressions/object/method-definition/private-name-early-error-async-gen.js(default)

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Sep 18, 2019

Author Member

This test (correctly) threw but for the wrong reason. I will open a [good first issue] for it after that this PR is merged.

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:duplicate-private branch from 8818dfd to 3b8e42c Sep 24, 2019
@buildsize

This comment has been minimized.

Copy link

buildsize bot commented Sep 24, 2019

File name Previous Size New Size Change
babel-preset-env.js 2.69 MB 2.74 MB 54.71 KB (2%)
babel-preset-env.min.js 1.63 MB 1.65 MB 25.6 KB (2%)
babel.js 2.9 MB 2.96 MB 54.72 KB (2%)
babel.min.js 1.6 MB 1.63 MB 25.61 KB (2%)
@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:duplicate-private branch 2 times, most recently from f2af0fd to 013d7fa Sep 24, 2019
@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:duplicate-private branch from 013d7fa to 1a86603 Dec 2, 2019
@JLHwung
JLHwung approved these changes Jan 9, 2020
Copy link
Contributor

JLHwung left a comment

👍for the code.

WDYT moving classScope to an extra scope handler like what we did in prodParam? It could get scopeHandler more focused then, especially private name is only allowed in class body so this.classScope.declarePrivateName looks straightforward to me.

// they have the same placement (static or not).
redefined =
!(differences & CLASS_ELEMENT_KIND_ACCESSOR) ||
differences & CLASS_ELEMENT_FLAG_STATIC;

This comment has been minimized.

Copy link
@jridgewell

jridgewell Jan 9, 2020

Member

This makes my head hurt. Why not split into multiple if-statements?

This comment has been minimized.

Copy link
@jridgewell

jridgewell Jan 9, 2020

Member

Or, just compare them in two separate steps:

// staticBit will be zero if both are static or both are instance.
const staticBit = (accessor & CLASS_ELEMENT_FLAG_STATIC) ^ (elementType & CLASS_ELEMENT_FLAG_STATIC);
// accessorBit will be zero if they are opposite types.
const accessorBit = (accessor & CLASS_ELEMENT_KIND_ACCESSOR) & (elementType & CLASS_ELEMENT_KIND_ACCESSOR);

redefined = !!(staticBit | accessorBit);

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jan 10, 2020

Author Member

Re-reading it made my head hurt too 😛

I'm rewriting it to this code:

        const oldStatic = accessor & CLASS_ELEMENT_FLAG_STATIC;
        const newStatic = elementType & CLASS_ELEMENT_FLAG_STATIC;

        const oldKind = accessor & CLASS_ELEMENT_KIND_ACCESSOR;
        const newKind = elementType & CLASS_ELEMENT_KIND_ACCESSOR;

        // The private name can be duplicated only if it is used by
        // two accessors with different kind (get and set), and if
        // they have the same placement (static or not).
        redefined = oldKind === newKind || oldStatic !== newStatic;
@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:duplicate-private branch from 1a86603 to f9edcba Jan 10, 2020
@nicolo-ribaudo nicolo-ribaudo merged commit 771c730 into babel:master Jan 10, 2020
4 of 5 checks passed
4 of 5 checks passed
build (13.x)
Details
test262 Workflow: test262
Details
Travis CI - Pull Request Build Passed
Details
build-standalone Workflow: build-standalone
Details
e2e Workflow: e2e
Details
@nicolo-ribaudo nicolo-ribaudo deleted the nicolo-ribaudo:duplicate-private branch Jan 10, 2020
@chicoxyzzy chicoxyzzy modified the milestone: Babel 8.x Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.