Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_js_analyze): implement the noUnreachableSuper rule #4017

Merged
merged 6 commits into from
Dec 21, 2022

Conversation

leops
Copy link
Contributor

@leops leops commented Dec 8, 2022

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 leops requested review from ematipico, xunilrj and a team as code owners December 8, 2022 13:59
@netlify
Copy link

netlify bot commented Dec 8, 2022

Deploy Preview for docs-rometools ready!

Name Link
🔨 Latest commit 92850cb
🔍 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.

@leops
Copy link
Contributor Author

leops commented Dec 8, 2022

!bench_analyzer

@github-actions
Copy link

github-actions bot commented Dec 8, 2022

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

Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name noUnreachableSuper suggests to me that the super call is unreachable but this isn't what the rule is testing for.

The rule is testing that super gets initialized on all paths. That's why I would recommend renaming the rule to noInvalidInitializedSuper

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. My concern with this is that it leads to an inflation of rules simply because of a technical detail. It would be elegant if multiple rules could be mapped to the same name to decouple the implementation from the rule name

/// Finishes building the function
pub fn finish(self) -> ControlFlowGraph<L> {
pub fn finish(mut self) -> ControlFlowGraph<L> {
// Append an implicit return instruction at the end of the function
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may help to explain why this is done and not just what it is doing

@leops leops added the A-Linter Area: linter label Dec 9, 2022
@leops leops force-pushed the feature/no-unreachable-super branch from 99c5e56 to e070741 Compare December 9, 2022 14:26
@leops
Copy link
Contributor Author

leops commented Dec 9, 2022

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 leops force-pushed the feature/no-unreachable-super branch from e070741 to d27c898 Compare December 19, 2022 16:47
@leops leops force-pushed the feature/no-unreachable-super branch 2 times, most recently from 9977d00 to a8ad55a Compare December 20, 2022 16:39
@leops leops force-pushed the feature/no-unreachable-super branch from a8ad55a to 5ad24c1 Compare December 21, 2022 08:36
@leops leops merged commit a69f962 into main Dec 21, 2022
@leops leops deleted the feature/no-unreachable-super branch December 21, 2022 09:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

noUnreachableSuper
3 participants