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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow duplicated nodes (only in tests output) #7149

Merged
merged 4 commits into from Jan 29, 2018

Conversation

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jan 3, 2018

Unfortunately this PR won't be easy to review 馃檨

Anyway, currently there are 604 failing tests 馃槤

I had to add some logic do skip validating nodes generated by regenerator-transform, since it isn't in this repository and it duplicates a lot of nodes.

cc @Andarist @existentialism

Closes #6375

@@ -68,7 +68,7 @@ function _foo() {
while (1) {
switch (_context3.prev = _context3.next) {
case 0:
_bar2 = function _bar2() {
_bar2 = function _ref2() {
_bar2 = _asyncToGenerator(

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jan 3, 2018

Author Member

I think that this was a bug, since _bar2 wasn't actually reassigned (because the variable was shadowed by the function)

@@ -172,6 +322,7 @@ function run(task) {
if (execCode) {
const execOpts = getOpts(exec);
result = babel.transform(execCode, execOpts);
checkDuplicatedNodes(result.ast);

This comment has been minimized.

Copy link
@Andarist

Andarist Jan 3, 2018

Member

i guess this might create quite some overhead, ideally we should not do this by default, maybe only on CI?

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jan 3, 2018

Author Member

I will disable it on normal make test at the end of the pr, when the tests are green.

This comment has been minimized.

Copy link
@hzoo

hzoo Jan 4, 2018

Member

is it actually slower?

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jan 5, 2018

Author Member

With the check:

make test-only  60,29s user 4,13s system 115% cpu 55,727 total

Without:

make test-only  57,88s user 3,98s system 115% cpu 53,697 total

We can run the check always.

if (isByRegenerator(node)) return;
if (nodes.has(node)) {
throw new Error(
"Do not reuse nodes. Use `t.clone` or `t.cloneDeep` to copy them.\n" +

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Jan 4, 2018

Member

(Q: when to not use cloneDeep?)

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jan 4, 2018

Author Member

When you are working with a literal, which doesn't have children

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Jan 4, 2018

Member

But if you are working with a literal, cloneDeep just won't find additional objects to recurse and so it won't recurse. Any reason why we can't just provide a single interface to reduce errors? Would the performance impact of the extra typeof check be too great?

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jan 4, 2018

Author Member

We can make clone behave as cloneDeep and special-case literals to avoid looping over their properties.

@Andarist

This comment has been minimized.

Copy link
Member

Andarist commented Jan 4, 2018

353 tests failing at the moment 馃帀

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Jan 5, 2018

@Andarist @Kovensky What do you think of t.cloneNode? It is more lightweight that t.cloneDeep and it has a fast path for identifiers (There were more t.clone than t.cloneNode, so we clone a lot of identifiers).
It has the advantage of not requiring the developer to choose between clone and cloneDeep.

Also, I think we used somewhere clone instead of cloneDeep, because the number of failing tests decreased by 2 馃帀

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Jan 5, 2018

We should add [skip ci] to our commits title to avoid running travis, since we know that it will fail for now.

@Jessidhia

This comment has been minimized.

Copy link
Member

Jessidhia commented Jan 5, 2018

@nicolo-ribaudo you mean defining a new cloneNode that calls the appropriate clone variant?

I'd still prefer to have only One True Cloning Function, with a check at the start to decide on fast paths; we could then make clone and cloneDeep forward to it.

EDIT: oh, you added it on a commit; time to read it

if (has(node, field)) {
newNode[field] = isNode(node[field])
? cloneNode(node[field])
: node[field];

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Jan 5, 2018

Member

This won't clone nodes that are inside a statement list (BlockStatement children, etc)

@@ -0,0 +1,35 @@
import { NODE_FIELDS } from "../definitions";

const has = Function.call.bind(Object.prototype.hasOwnProperty);

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Jan 5, 2018

Member

(call is technically in Function.prototype but it works I guess)

type: node.type,
}: any): T);
for (const field in NODE_FIELDS[node.type]) {
if (has(node, field)) {

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Jan 5, 2018

Member

I'd tell you to use Object.keys but I assume you did this for speed

const isNode = obj => obj && has(NODE_FIELDS, obj.type);

export default function cloneNode<T: Object>(node: T): T {
if (!node) return node;

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Jan 5, 2018

Member

If T extends Object it can't be falsy, unless it's document.all :trollface:

(I guess this is defensive programming)

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jan 5, 2018

Author Member

Oh lol, I copied lines 6-7 from clone. I will leave the check here, because plug-in authors might not use Flow.

const has = Function.call.bind(Object.prototype.hasOwnProperty);
const isNode = obj => obj && has(NODE_FIELDS, obj.type);

export default function cloneNode<T: Object>(node: T): T {

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Jan 5, 2018

Member

I'd say it would be better to extend BabelNode instead...... but actually reading the BabelNode definition and how it's used makes it a bit doubtful.

For example, in TypeScript, almost every valid node (written as an object literal) you gave it would get caught in excess property checks for weak types. It'd probably be better to make BabelNode be a generic, where the generic argument is the node type literal string.

But I digress and is outside the scope of this change.

if (!node) return node;

// Special-case identifiers since they are the most cloned nodes.
if (node.type === "Identifier") {

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Jan 5, 2018

Member

Maybe better to save type to a local var (const { type } = node) since you reuse it at least 3 times

}
}

newNode.loc = node.loc || null;

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Jan 5, 2018

Member

Since you're now specifically only copying known AST keys, this is missing comments.

@Andarist

This comment has been minimized.

Copy link
Member

Andarist commented Jan 5, 2018

@nicolo-ribaudo do you plan to remove clone and cloneDeep altogether?

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Jan 5, 2018

Remove or deprecate with a nice warning message

@Andarist

This comment has been minimized.

Copy link
Member

Andarist commented Jan 5, 2018

If you gonna deprecate them, which I guess is fine, considering scope of the babel and its users count, I'd propose to have them just proxy to cloneNode with a deprecation warning - so actually we'd have only a single implementation for this stuff.

I must also add, that I really like this change - was really hard to determine which one (clone vs cloneDeep) should be used in certain cases - especially when something could be an Identifier once but also i.e. a MemberExpression in other cases

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Jan 5, 2018

If you gonna deprecate them, which I guess is fine, considering scope of the babel and its users count, I'd propose to have them just proxy to cloneNode with a deprecation warning - so actually we'd have only a single implementation for this stuff.

Done 馃憤

In the test output there is the depecation message twice, because regenerator uses t.cloneDeep [1]

[1] https://github.com/facebook/regenerator/blob/aca60c7bfae712fe4a1b1264415b73a87df6ca0e/packages/regenerator-transform/src/replaceShorthandObjectMethod.js#L56-L75

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Jan 7, 2018

24 failing tests 馃帀

@hzoo hzoo added the Priority: High label Jan 7, 2018
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Jan 7, 2018

Zero tests failing locally 馃帀

@Andarist I'll rebase this commits to resolve the merge conflicts and squash them. Are you ok if I squash them like this?

  1. Disallow duplicated nodes in tests output (#7149) (author: nicolo-ribaudo)
  2. Add t.cloneNode and deprecate t.clone and t.cloneDeep (#7149) (author: nicolo-ribaudo)
  3. Fix reused nodes - part 1 (#7149) (author: Andarist)
  4. Fix reused nodes - part 2 (#7149) (author: nicolo-ribaudo)
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Jan 7, 2018

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

@babel babel deleted a comment from babel-bot Jan 7, 2018
@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:duplicated-nodes branch from d1a329a to def83a3 Jan 7, 2018
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jan 7, 2018
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jan 7, 2018
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jan 7, 2018
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jan 7, 2018
@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:duplicated-nodes branch from def83a3 to 79ef356 Jan 7, 2018
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jan 7, 2018
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jan 7, 2018
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jan 7, 2018
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jan 7, 2018
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Jan 7, 2018

I've gone ahead and squashed. The original commits are at https://github.com/nicolo-ribaudo/babel/tree/duplicated-nodes-old.

There were so many conflicts when reordering commits that I don't know how I managed not to break everything* 馃槅

* On my PC there is part 1 and then part 2, but here they are swapped 馃

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jan 11, 2018
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jan 11, 2018
@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Jan 23, 2018

Let's try to land this asap? @loganfsmyth thoughts?

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:duplicated-nodes branch from a02297f to c7a2d35 Jan 29, 2018
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Jan 29, 2018

When merging, please don't squash to keep both @Andarist and I as the commit authors.

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Jan 29, 2018

You can merge if it's good now 馃憤

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Jan 29, 2018

I'm waiting for travis and circle ci to finish 馃憤

@nicolo-ribaudo nicolo-ribaudo merged commit 252ea5a into babel:master Jan 29, 2018
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 84.17% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
nicolo-ribaudo added a commit that referenced this pull request Jan 29, 2018
@Andarist

This comment has been minimized.

Copy link
Member

Andarist commented Jan 29, 2018

wohoooo 馃帀

@nicolo-ribaudo nicolo-ribaudo deleted the nicolo-ribaudo:duplicated-nodes branch Jan 29, 2018
@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Jan 29, 2018

amazing - now we should figure out how to make this a thing in all the 3rd party plugins with our plugin cli tool 馃槃

aminmarashi-binary added a commit to aminmarashi-binary/babel that referenced this pull request Mar 17, 2018
aminmarashi-binary added a commit to aminmarashi-binary/babel that referenced this pull request Mar 17, 2018
aminmarashi-binary added a commit to aminmarashi-binary/babel that referenced this pull request Mar 17, 2018
aminmarashi-binary added a commit to aminmarashi-binary/babel that referenced this pull request Mar 17, 2018
@lock lock bot added the outdated label May 2, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can鈥檛 perform that action at this time.