feat(eslint-plugin): add rule call-super-on-override

[mahdi-farnia]

PR Checklist

• Addresses an existing open issue: fixes Rule proposal: checks if you remembered to call super.method() #684
• That issue was marked as accepting prs
• Steps in CONTRIBUTING.md were taken

Overview

Before:

1. Warning expected to note that overridden method should call it's super method to prevent missing something!

// Trivia example
class Foo {
  dealloc(): void {
    memory.dealloc();
  }
}

class Bar extends Foo {
  override dealloc(): void {
    console.log("Deallocated"); // note that memory.dealloc() didn't call
  }
}

After:

// Trivia example
class Foo {
  dealloc(): void {
    memory.dealloc();
  }
}

class Bar extends Foo {
  override dealloc(): void {
    console.log("Deallocated"); // note that memory.dealloc() didn't call
    // ^^^ missingSuperMethodCall warning
  }
}

This checking can be disabled via option ignoreMethods: true

2. Also ordering may be important:

Before:

// Trivia example on drawing a circle with Canvas API
class MyCircle extends Circle {
  override draw(): void {
    this.changeColor("blue");
    super.draw(); // <-- ordering matters here
  }
}

After:

// Trivia example on drawing a circle with Canvas API
class MyCircle extends Circle {
  override draw(): void {
    this.changeColor("blue");
    super.draw();
    // ^^^ topLevel warning
  }
}

This options is disabled by default and can be enabled by topLevel: true

Closes #684

Note about local git history by me: I rewrite it again after one month, and i figured out that the last clone is outdated and i cloned the latest repo and just copied & pasted the file.
Because of that there is no history available for this project.

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit b7e9812. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

🟥 Failed Commands
nx run-many --target=lint --parallel

✅ Successfully ran 45 targets

• Node 18 - nx test eslint-plugin
• Node 14 - nx test eslint-plugin --coverage=false
• Node 18 - nx test utils
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx test visitor-keys
• Node 18 - nx run-many --target=build --parallel --exclude website
• nx run-many --target=build --parallel --exclude website
• Node 18 - nx test type-utils
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx test typescript-estree
• Node 18 - nx run-many --target=build --parallel --exclude website
• nx run-many --target=typecheck --parallel
• Node 14 - nx test eslint-plugin-internal --coverage=false
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx test eslint-plugin-tslint
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx test parser --coverage=false
• Node 14 - nx test typescript-estree --coverage=false
• Node 14 - nx test type-utils --coverage=false
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx test utils --coverage=false
• Node 18 - nx test ast-spec
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx test eslint-plugin-tslint --coverage=false
• Node 18 - nx test parser
• Node 14 - nx test visitor-keys --coverage=false
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx test scope-manager --coverage=false
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx test ast-spec --coverage=false
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx test eslint-plugin-internal
• Node 18 - nx run-many --target=build --parallel --exclude website
• nx run-many --target=build --parallel --exclude website
• Node 18 - nx test scope-manager
• Node 18 - nx run-many --target=build --parallel --exclude website
• nx run-many --target=build --parallel --exclude website
• nx run-many --target=build --parallel --exclude website

---

Sent with 💌 from NxCloud.

[typescript-eslint]

Thanks for the PR, @mahdi-farnia!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

[netlify]

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	2b97414
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6410b7554847d10008e3467f
😎 Deploy Preview	https://deploy-preview-5848--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[codecov]

Codecov Report

Merging #5848 (93e9c16) into main (9eafd87) will increase coverage by 0.01%.
The diff coverage is 90.32%.

❗ Current head 93e9c16 differs from pull request most recent head 2b97414. Consider uploading reports for the commit 2b97414 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5848      +/-   ##
==========================================
+ Coverage   85.15%   85.16%   +0.01%     
==========================================
  Files         383      384       +1     
  Lines       13026    13057      +31     
  Branches     3839     3851      +12     
==========================================
+ Hits        11092    11120      +28     
  Misses       1572     1572              
- Partials      362      365       +3     

Flag	Coverage Δ
unittest	85.16% <90.32%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../eslint-plugin/src/rules/call-super-on-override.ts	90.32% <90.32%> (ø)

[JoshuaKGoldberg]

Great start! I don't have time to work on this now, so leaving a review for now as usual. As discussed in #684 (comment), anybody can feel free to send a new PR addressing the review. ❤️

[JoshuaKGoldberg]

Getting closer, thanks for continuing to work on this! Unless someone in @typescript-eslint/triage-team disagrees with me, I think we'll want type checking in the rule?

[JoshuaKGoldberg]

[Refactor] Aha, a duplication of information! methodNameIsLiteral will always be true if methodNameIsNull is true. So this is using two variables to store a single piece of info. I'd suggest using either a union type or enum to consolidate down to one variable.

For example, the enum form might look something like this (but maybe with different names, I haven't thought too hard about these):

type MethodNameVariant = 'identifier' | 'non-null-literal' | 'null-literal';

[JoshuaKGoldberg]

node.key as TSESTree.Literal

[Refactor] This as assertion isn't always going to be correct. The TSESTree types are set up to represent all the possible values of properties. The fact that an as was needed to get this code to type check means that the code isn't handling some case that it needs to handle.

Removing the as gives:

Property 'value' does not exist on type 'FunctionExpression | ArrayExpression | ArrayPattern | ArrowFunctionExpression | AssignmentExpression | ... 35 more ... | YieldExpression'.
  Property 'value' does not exist on type 'FunctionExpression'.

...which is true. What if the node's key is a function expression?

class InvalidSample {
  [function () { }]() {
    console.log("Yippee!")
  }
}

This code is wacky (and causes a TypeScript complaint) but someone in the wild might try to run this rule on that code.

Change request: remove all the as assertions from this file, and explicitly handle those odd node edge cases.

Note too that we require high unit test coverage (generally, ~100% if possible) for new rule tests. So this'll need unit tests added for any new logic. You can always run yarn test from packages/eslint-plugin to generate a testing coverage report at packages/eslint-plugin/coverage/lcov-report/index.html (or see the Codecov PR comment for an online viewer).

[JoshuaKGoldberg]

A lot of consumers don't use noImplicitOverride. I think we'll still want the rule to be able to work for them... which means it'll have to use rule type checking when there isn't an override operator on a method.

Roughly, that means:

1. Grabbing the type of the extended class
2. Checking whether it has a method / function property under the same name

[JoshuaKGoldberg]

Let's keep conversations threaded, please, so it's easier to have multiple at a time & mark them as open or resolved.

As to how to grab the type of the extended class: you'll need to use the type checker. Have you read our resources on doing that?

• https://typescript-eslint.io/blog/asts-and-typescript-eslint
• https://sourcegraph.com/notebooks/Tm90ZWJvb2s6MTA2OA==
• https://typescript-eslint.io/custom-rules#typed-rules

[JoshuaKGoldberg]

👋 @mahdi-farnia! Just checking in, is this still something you have time for? No worries if not - I just don't want to leave it hanging.

[mahdi-farnia]

I'll name this laziness of myself

Sorry about that
I am going to check it tonight ( My country has 12:30 hours difference from US )

I forgot resolving issues after my college exams...🤦‍♂️

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 2b97414. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

🟥 Failed Commands
Node 18 - nx test eslint-plugin

✅ Successfully ran 24 targets

• Node 18 - nx test typescript-estree
• Node 18 - nx test type-utils
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx test utils
• Node 18 - nx test visitor-keys
• Node 18 - nx test parser
• Node 18 - nx test scope-manager
• Node 18 - nx test eslint-plugin-tslint
• nx run-many --target=lint --parallel
• nx run-many --target=typecheck --parallel
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx test eslint-plugin-tslint --coverage=false
• nx run-many --target=build --parallel --exclude website
• Node 18 - nx test eslint-plugin-internal
• Node 14 - nx test ast-spec --coverage=false
• Node 14 - nx test type-utils --coverage=false
• Node 14 - nx test parser --coverage=false
• Node 14 - nx test utils --coverage=false
• Node 14 - nx test visitor-keys --coverage=false
• Node 14 - nx test scope-manager --coverage=false
• Node 18 - nx test ast-spec
• Node 14 - nx test typescript-estree --coverage=false
• Node 14 - nx test eslint-plugin-internal --coverage=false
• Node 14 - nx test eslint-plugin --coverage=false

---

Sent with 💌 from NxCloud.

[mahdi-farnia]

I need help!
How can grabbing the type of the extended class(es)?
Any article or guid?
@JoshuaKGoldberg

[mahdi-farnia]

Just note that i did not make any progress (for now).
I just applied some suggested changes

I already using astexplorer and ts-ast-viewer

Is there any other tool?

[JoshuaKGoldberg]

I responded in this thread: #5848 (comment)