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

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

Merged
merged 8 commits into from Aug 26, 2021

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Aug 6, 2021

Prerequisites checklist

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 nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion chore This change is not user-facing labels Aug 6, 2021
@nzakas nzakas linked an issue Aug 6, 2021 that may be closed by this pull request
8 tasks
@nzakas
Copy link
Member Author

nzakas commented Aug 7, 2021

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:

graphviz(1)
graphviz

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
Copy link
Member

mdjermanovic commented Aug 9, 2021

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
Copy link
Member Author

nzakas commented Aug 9, 2021

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 nzakas changed the title Chore: Code path analysis for class fields (fixes #14343) Update: Code path analysis for class fields (fixes #14343) Aug 9, 2021
@nzakas
Copy link
Member Author

nzakas commented Aug 9, 2021

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
Copy link
Member Author

nzakas commented Aug 9, 2021

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

class Foo {
    [a] = b;
}

graphviz(3)

graphviz(4)

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

@mdjermanovic
Copy link
Member

mdjermanovic commented Aug 9, 2021

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
Copy link
Member Author

nzakas commented Aug 10, 2021

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.

graphviz(5)
graphviz(6)

@mdjermanovic mdjermanovic added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint and removed chore This change is not user-facing labels Aug 10, 2021
Copy link
Member

@mdjermanovic mdjermanovic left a comment

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
Copy link
Member Author

nzakas commented Aug 23, 2021

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
Copy link
Member

mdjermanovic commented Aug 23, 2021

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
Copy link
Member Author

nzakas commented Aug 24, 2021

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

@nzakas
Copy link
Member Author

nzakas commented Aug 25, 2021

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.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Awesome, thanks! origin makes sense to me.

LGTM, I left only two minor notes.

tests/lib/linter/code-path-analysis/code-path.js Outdated Show resolved Hide resolved
lib/linter/code-path-analysis/code-path.js Outdated Show resolved Hide resolved
nzakas and others added 2 commits Aug 26, 2021
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@mdjermanovic mdjermanovic merged commit 05ca24c into master Aug 26, 2021
13 checks passed
@mdjermanovic mdjermanovic deleted the class-fields-code-path branch Aug 26, 2021
@mdjermanovic
Copy link
Member

mdjermanovic commented Aug 26, 2021

Merged, thanks for taking on this task!

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Feb 23, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support Class Fields
2 participants