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

Code path segments start outside of a class and end in a class instance field initializer. #14317

Closed
andrey-tyukin-sonarsource opened this issue Apr 12, 2021 · 2 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly experimental syntax This change is related to experimental syntax (stage 3 or below) repro:needed
Projects

Comments

@andrey-tyukin-sonarsource
Copy link

Tell us about your environment

  • ESLint Version: 7.24.0
  • Node Version: v14.16.1

What parser are you using?

Example below uses babel-eslint; The issue is parser-independent, same holds for typescript-eslint.

Please show your full configuration:

Configuration These are the used dependencies:
  "dependencies": {
    "babel-eslint": "^10.1.0",
    "eslint": "^7.24.0"
  }

The example below is not concerned with any specific rule; a minimal configuration is included directly in the code snippet.

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

This is an excerpt from this repository with a full reproducible example. It defines a single rule that simply prints out all the node- and codePath-related events emitted by ESLint:

const codeSnippet = `
  var marker_01 = '';
  class A {}
  class B extends A {
    p = (marker_02 = '') ? (marker_03 = '') : (marker_04 = '');
    constructor() {
      marker_05 = '';
      super();
      marker_06 = '';
    }
  }
  var marker_07 = '';
`;
const eslint = require('eslint');
const babelEslint = require('babel-eslint');
const linter = new eslint.Linter();
const rule = {
  create: function(context) {
    var indentation = 0;
    function print(msg) {
      console.log(' '.repeat(indentation) + msg);
    }
    return {
      onCodePathStart(codePath) {
        print(`PATH_START ${codePath.id}`);
      },
      onCodePathEnd(codePath) {
        print(`PATH_END   ${codePath.id}`);
      },
      onCodePathSegmentStart(codePathSegment) {
        print(`SEGMENT_START ${codePathSegment.id}`);
      },
      onCodePathSegmentEnd(codePathSegment) {
        print(`SEGMENT_END   ${codePathSegment.id}`);
      },
      ['*'](node) {
        print(`NODE_START ${node.type} ${node.type === 'Identifier' ? node.name : ''}`);
        indentation++;
      },
      ['*:exit'](node) {
        indentation--;
        print(`NODE_END   ${node.type} ${node.type === 'Identifier' ? node.name : ''}`);
      }
    };
  }
};

const parsingResult = babelEslint.parseForESLint(codeSnippet);
const sourceCode = new eslint.SourceCode({ text: codeSnippet, ...parsingResult });
linter.defineRule('x', rule);
linter.verify(sourceCode, { rules: { x: ['error'] } }, 'filePath.js');

Command used to run the example (see Readme in the linked repo):

node index.js 2               # See linked repository for the full example

What did you expect to happen?

I expected that the initializer of the class field p is processed after the constructor is entered, and before constructor is exited. In particular, I expected that the control flow constructs inside of the initializer do not interfere with control flow constructs outside of the class. The expected sequence of events should look somewhat like this (the position of the steps in bold is crucial):

  • start of top-level code path
  • start of top-level code path segment s1_1
  • ...
  • start of class body
  • start of constructor code path
  • start of constructor code path segment s2_1
  • ...
  • super call
  • start of class property
  • marker_02
  • end of path segment s2_1
  • (... more path segments with marker_03 and marker_04...)
  • end of constructor code path
  • end of class body
  • ...
  • end of top-level code path

What actually happened? Please copy-paste the actual, raw output from ESLint.

These are the events emitted by ESLint, indented for legibility, commented at the crucial points:

PATH_START s1
SEGMENT_START s1_1                               // start segment s1_1
NODE_START Program 
 NODE_START VariableDeclaration 
  NODE_START VariableDeclarator 
   NODE_START Identifier marker_01
   NODE_END   Identifier marker_01
   NODE_START Literal 
   NODE_END   Literal 
  NODE_END   VariableDeclarator 
 NODE_END   VariableDeclaration 
 NODE_START ClassDeclaration 
  NODE_START Identifier A
  NODE_END   Identifier A
  NODE_START ClassBody 
  NODE_END   ClassBody 
 NODE_END   ClassDeclaration 
 NODE_START ClassDeclaration 
  NODE_START Identifier B
  NODE_END   Identifier B
  NODE_START ClassBody                          // start body of class B
   NODE_START ClassProperty 
    NODE_START Identifier p
    NODE_END   Identifier p
    NODE_START ConditionalExpression 
     NODE_START AssignmentExpression 
      NODE_START Identifier marker_02
      NODE_END   Identifier marker_02
      NODE_START Literal 
      NODE_END   Literal 
     NODE_END   AssignmentExpression 
     SEGMENT_END   s1_1                        // unexpected end of s1_1
     SEGMENT_START s1_2
     NODE_START AssignmentExpression 
      NODE_START Identifier marker_03
      NODE_END   Identifier marker_03
      NODE_START Literal 
      NODE_END   Literal 
     NODE_END   AssignmentExpression 
     SEGMENT_END   s1_2
     SEGMENT_START s1_3
     NODE_START AssignmentExpression 
      NODE_START Identifier marker_04
      NODE_END   Identifier marker_04
      NODE_START Literal 
      NODE_END   Literal 
     NODE_END   AssignmentExpression 
     SEGMENT_END   s1_3
     SEGMENT_START s1_4
    NODE_END   ConditionalExpression 
   NODE_END   ClassProperty 
   NODE_START MethodDefinition 
    NODE_START Identifier constructor
    NODE_END   Identifier constructor
    PATH_START s2                             // start of constructor segment
    SEGMENT_START s2_1
    NODE_START FunctionExpression 
     NODE_START BlockStatement 
      NODE_START ExpressionStatement 
       NODE_START AssignmentExpression 
        NODE_START Identifier marker_05
        NODE_END   Identifier marker_05
        NODE_START Literal 
        NODE_END   Literal 
       NODE_END   AssignmentExpression 
      NODE_END   ExpressionStatement 
      NODE_START ExpressionStatement 
       NODE_START CallExpression 
        NODE_START Super                     // super call
        NODE_END   Super 
       NODE_END   CallExpression 
      NODE_END   ExpressionStatement 
      NODE_START ExpressionStatement    // expected position of instance field initializers
       NODE_START AssignmentExpression 
        NODE_START Identifier marker_06
        NODE_END   Identifier marker_06
        NODE_START Literal 
        NODE_END   Literal 
       NODE_END   AssignmentExpression 
      NODE_END   ExpressionStatement 
     NODE_END   BlockStatement 
    NODE_END   FunctionExpression 
    SEGMENT_END   s2_1
    PATH_END   s2
   NODE_END   MethodDefinition 
  NODE_END   ClassBody 
  NODE_START Identifier A
  NODE_END   Identifier A
 NODE_END   ClassDeclaration 
 NODE_START VariableDeclaration 
  NODE_START VariableDeclarator 
   NODE_START Identifier marker_07
   NODE_END   Identifier marker_07
   NODE_START Literal 
   NODE_END   Literal 
  NODE_END   VariableDeclarator 
 NODE_END   VariableDeclaration 
NODE_END   Program 
SEGMENT_END   s1_4
PATH_END   s1

The problem is that the initializer of the property p is processed before the constructor is entered. Instead of modifying the control flow inside of the constructor, the ternary expression interferes with the code path segment s1_1, which shouldn't even be on the same codePath.

Steps to reproduce this issue:

Using the linked repo with MCVE

  1. Clone
  2. npm install
  3. node index.js 2 # Or "1" for another example

What I've attempted to solve the issue

I've attempted to modify the AST before the control flow analysis phase.

More specifically:

  • I modified the class declaration / class expression nodes
  • I collected all class properties
  • I attempted to find a constructor, if there was one
  • In case there was a constructor, I attempted to find a super(...)-call within it
  • In case there was no constructor, I generated a synthetic one.
  • I moved all the class instance field initializers and assignments either to the start of the constructor, or right after the super-call (approximately as described here).

This was partially successful (the blocks looked a bit better, the initializers did not interfere with code path segments outside of the constructor), but it caused some unexpected changes during execution of other rules.

Are you willing to submit a pull request to fix this bug?
Yes*

(* Assuming that the above approach has a chance of not leading to any unexpected consequences)

@andrey-tyukin-sonarsource andrey-tyukin-sonarsource added bug ESLint is working incorrectly repro:needed labels Apr 12, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Apr 12, 2021
@mdjermanovic mdjermanovic moved this from Needs Triage to Blocked in Triage Apr 12, 2021
@mdjermanovic mdjermanovic added the experimental syntax This change is related to experimental syntax (stage 3 or below) label Apr 12, 2021
@mdjermanovic
Copy link
Member

Hi @andrey-tyukin-sonarsource, thanks for the issue!

ESLint's code path analysis currently doesn't support class fields. We'll start with adding support for this syntax when it reaches stage 4 (which may happen soon, according to the agenda for the upcoming Ecma TC39 meeting).

@mdjermanovic
Copy link
Member

I expected that the control flow constructs inside of the initializer do not interfere with control flow constructs outside of the class.

This is fixed by #14886, released in ESLint v8.0.0-beta.1. Each class field initializer is now treated as an implicit function and therefore starts a separate CodePath.

We also added CodePath#origin property ("program", "function", or "class-field-initializer") to help with differentiating multiple code paths started on the same node.

For example class Foo { x = () => {} } - this arrow function node starts a "class-field-initializer" code path, and then right away an inner "function" code path.

I expected that the initializer of the class field p is processed after the constructor is entered, and before constructor is exited.

Our code path analysis doesn't provide this info. We track the flow inside code paths (i.e. between code path segments) but not between code paths.

Triage automation moved this from Blocked to Complete Sep 2, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Mar 2, 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 Mar 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly experimental syntax This change is related to experimental syntax (stage 3 or below) repro:needed
Projects
Archived in project
Triage
Complete
Development

No branches or pull requests

2 participants