Update: Code path analysis for class fields (fixes #14343)

[nzakas]

Prerequisites checklist

• I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:

What changes did you make? (Give an overview)

I added tests for code path analysis to show that analysis is working correctly for class fields. No other changes were necessary as I believe the standard traversal gives the code path analyzer all of the information it needs.

I started by adding a test that showed how we currently traverse an object literal with a property that is initialized by a conditional statement because this is the closest analog to how code path analysis should work with class fields.

I then added a test showing that a class field initialized with a conditional statement creates the correct code path, as it basically matches what happened in the object literal case.

Is there anything you'd like reviewers to focus on?

Am I missing anything?

[nzakas]

So I took a look at the code path analysis for a similar pattern:

function Foo() {
    this.a = b ? c : d
};

new Foo()

The result is:

initial->s2_1->s2_2->s2_4;
s2_1->s2_3->s2_4->final;
initial->s1_1->final;

Which results in the following dot graphs:

Similar to class fields, this.a isn't initialized until new Foo() is called, but we don't tie those two calls together currently. If that's the case, it seems like the same would apply to class fields in an example like this:

class Foo {
    a = b ? c : d;
}

new Foo();

So do we have a bug in our existing code path analysis? Or are the class fields different in some way that I'm not seeing?

(Again, I really don't understand how any of this works, so please feel free to point out how I'm misunderstanding this.)

[mdjermanovic]

A separate code path (which we currently create only for functions, incl. methods) indicates that the code inside isn't executed in the same flow as the surrounding code, but we don't track function calls to connect code paths. That info isn't provided by our code path analysis, so new Foo() isn't tied to the constructor of class Foo. Our code path analysis only splits the code into unconnected code paths (Program and functions) and then splits code paths into code path segments (which do provide detailed info about the flow in each code path).

A;

class B extends C {
    [D] = E + F;
}

G;

A -> B -> C -> D -> G is one execution flow, they're all evaluated one after another. AST traversal also goes through E + F between D and G, but it isn't evaluated between D and G, so it's not part of the same flow.

The specification defines initializers as functions https://tc39.es/ecma262/#sec-runtime-semantics-classfielddefinitionevaluation.

For example:

class Foo {
   x = 0;  
   y = this.x + 1;
}

is roughly the same as:

function xInitializer() {
    return 0;
}

function yInitializer() {
    return this.x + 1;
}

class Foo {
    constructor() {
        Object.defineProperty(this, "x",  { value: xInitializer.call(this) /*, ... */ });
        Object.defineProperty(this, "y",  { value: yInitializer.call(this) /*, ... */ });
    }
}

I don't think our code path analysis can provide too advanced info about the execution flow of class fields, like the one proposed in #14317, but it seems reasonable to at least create a separate code path for each class field initializer, i.e. to treat E + F as a function.

[nzakas]

Gotcha. That makes sense, thanks for explaining. I'm still not sure I know how to do that, but I'll dig in a bit.

[nzakas]

A note for myself: The challenge here is to treat the right side of PropertyDefinition as a FunctionExpression even though it isn't one. Otherwise, normal traversal applies. So, when we hit PropertyDefinition, we need to traverse into the key but not into the value. The value side needs to be a separate code path.

[nzakas]

So here is what I have so far. For this code:

class Foo {
    [a] = b;
}

I know this isn't correct, but how close does it look?

[mdjermanovic]

This definitely looks like the right direction!

It could be actually already correct although it doesn't appear so. Maybe just debug-helpers don't handle this example well so it doesn't look good by looking at the graphs (note that FunctionDeclaration in #14886 (comment) also appears both in its own code path and the outer code path; Identifier doesn't have child nodes, this code optimizes the output in such cases, but now when a lone Identifier represents a separate code path it produces a weird output with :enter in the outer code path).

[nzakas]

Okay, I think I've got it! I still don't 100% understand everything that's going on, but this commit seems to work the way (I think) we're expecting.

[mdjermanovic]

LGTM! This works as I would expect it to.

The only thing that might be confusing and difficult to handle in rules is that for code such as class Foo { a = () => {} } the onCodePathStart is raised twice with the same node argument (the arrow function), so we might want to add CodePath#type property which could be used to make a distinction between the initializer code path and function's own code path.

[nzakas]

so we might want to add CodePath#type property which could be used to make a distinction between the initializer code path and function's own code path.

Can you explain this a bit more? I'm not understanding what you're suggesting here.

[mdjermanovic]

My suggestion is that we add type property to CodePath objects. Value would be one of "program", "function", "class-field-initializer".

Rules are using code path analysis through events, like onCodePathStart(codePath, node).

Before introducing class fields, it was clear what each code path refers to, by node.type. If node.type === "Program", then the code path represents top-level code, otherwise node is a function node and the code path represents that function's code.

With class fields initializers, which don't have a dedicated wrapper node in the AST, onCodePathStart will be ambiguous when PropertyDefinition.value is a function node.

class Foo { 
    a = () => {};
}

For the above code, onCodePathStart(codePath, node) will be called two times with the same ArrowFunctionExpression node, first time representing the initializer's code path, second time representing function's own code path, but the rule can't easily know which is which. For that reason, codePath.type feels like helpful additional info for rules.

[nzakas]

Ah gotcha, thanks for explaining. That sounds like a good way to differentiate the types of code paths. I’ll work on that.

[nzakas]

I added an origin property to CodePath. This seemed to fit better than type, because the code path's shape or behavior doesn't change based on why it was created. Let me know if that makes sense.

[mdjermanovic]

Awesome, thanks! origin makes sense to me.

LGTM, I left only two minor notes.

[mdjermanovic]

Merged, thanks for taking on this task!