tools icon indicating copy to clipboard operation
tools copied to clipboard

feat(rome_js_analyze): implement the noUnreachableSuper rule

Open leops opened this issue 3 years ago • 4 comments

Summary

Fixes #3481

This PR implements a rule called noUnreachableSuper that performs an analysis on the control flow graph of class constructors of derived classes to ensure the following:

  • The super() expression must be executed exactly once on all paths through the constructor
  • All expressions accessing this must be preceded by a call to super()

The current name of the rule is derived from the existing noUnreachable rule that's also based on the control flow graph, but isn't very representative of what the rule actually does (it's technically a mix of some of the concerns implemented by constructor-super and no-this-before-super in ESLint).

The PR also modifies the noInvalidConstructorSuper that was previously responsible for checking that derived classes we correctly calling super() since the CFG-based analysis can perform this check more precisely. Finally, I've also modified the ControlFlowGraph struct to expose a node field containing the node this instance of the control flow graph was built for (in JavaScript this will be a JsAnyControlFlowRoot)

Test Plan

I've added new test cases for the various diagnostics emitted by the rule.

leops avatar Dec 08 '22 13:12 leops

Deploy Preview for docs-rometools ready!

Name Link
Latest commit 92850cbe0f67d188757ee40aabee7217ffaca713
Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/63a2c8ff85df16000894eaaf
Deploy Preview https://deploy-preview-4017--docs-rometools.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Dec 08 '22 13:12 netlify[bot]

!bench_analyzer

leops avatar Dec 08 '22 14:12 leops

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00      2.4±0.01ms     4.9 MB/sec    1.01      2.4±0.01ms     4.9 MB/sec
analyzer/index.js         1.00      6.1±0.01ms     5.4 MB/sec    1.01      6.1±0.01ms     5.3 MB/sec
analyzer/lint.ts          1.00      2.8±0.01ms    15.0 MB/sec    1.02      2.8±0.00ms    14.8 MB/sec
analyzer/parser.ts        1.00      7.1±0.03ms     6.9 MB/sec    1.01      7.1±0.03ms     6.8 MB/sec
analyzer/router.ts        1.00      2.5±0.01ms    12.2 MB/sec    1.00      2.5±0.04ms    12.2 MB/sec
analyzer/statement.ts     1.00      6.8±0.01ms     5.2 MB/sec    1.02      6.9±0.04ms     5.1 MB/sec
analyzer/typescript.ts    1.00      9.9±0.03ms     5.5 MB/sec    1.03     10.2±0.07ms     5.4 MB/sec

github-actions[bot] avatar Dec 08 '22 14:12 github-actions[bot]

Other than that it's my impression that the main reason that noUnreachableSuper and noInvalidCosntructorSuper are two different rules because they query for different nodes.

I think the only reason those are two different rules is because it was easier to only implement the syntax-based check first, and the more complex CFG-based analysis later. The existing logic in noInvalidConstructorSuper could be pulled into noUnreachableSuper to merge them into a single rule, in fact this is the reason I held back on stabilizing noInvalidConstructorSuper in v11 to potentially allow this merge to be done without being a breaking change. The only important caveat to doing this is that noUnreachableSuper only runs if a control flow graph can be emitted for the constructor, which requires the function has no statement-level syntax error. This means the noInvalidConstructorSuper rule would no longer be run if the constructor contains a syntax error.

leops avatar Dec 09 '22 14:12 leops