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

Fix: no-useless-constructor (#13830) #13842

Merged

Conversation

AriPerkkio
Copy link
Contributor

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[x] 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
[ ] Other, please explain:

Fixes #13830

  • Do not crash on parsers which do not require class constructor to have a body

What changes did you make? (Give an overview)

Old implementation relies that class constructor has body. Added a check to verify this before accessing the object in order to prevent null pointer.

Is there anything you'd like reviewers to focus on?

As discussed on the ticket, it seems to be common practice to disable this rule when using typescript-parser. However I would rather avoid linter crash when this rule is enabled with specific parsers. Currently users will not see other erroneous cases in the same file due to linter crash.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Nov 14, 2020
@eslint-deprecated
Copy link

Hi @AriPerkkio!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The issue reference must be formatted as follows:

    If the pull request addresses an issue, then the issue number should be mentioned at the end. If the commit doesn't completely fix the issue, then use (refs #1234) instead of (fixes #1234).

    Here are some good commit message summary examples:

    Build: Update Travis to only test Node 0.10 (refs #734)
    Fix: Semi rule incorrectly flagging extra semicolon (fixes #840)
    Upgrade: Esprima to 1.2, switch to using comment attachment (fixes #730)
    

Read more about contributing to ESLint here

@AriPerkkio AriPerkkio force-pushed the fix/no-useless-constructor-crash branch from 012a7b4 to d8a957e Compare November 14, 2020 09:45
@eslint-deprecated
Copy link

Hi @AriPerkkio!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The length of the commit message must be less than or equal to 72

Read more about contributing to ESLint here

@AriPerkkio AriPerkkio force-pushed the fix/no-useless-constructor-crash branch from d8a957e to b944314 Compare November 14, 2020 09:47
@mdjermanovic mdjermanovic added 3rd party plugin This is an issue related to a 3rd party plugin, config, or parser bug ESLint is working incorrectly evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Nov 15, 2020
@mdjermanovic
Copy link
Member

As discussed on the ticket, it seems to be common practice to disable this rule when using typescript-parser. However I would rather avoid linter crash when this rule is enabled with specific parsers. Currently users will not see other erroneous cases in the same file due to linter crash.

Makes sense to me, I support this since it's a small change in the code.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Not crashing is always good.

@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Nov 18, 2020
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit d2239a1 into eslint:master Nov 18, 2020
@mdjermanovic
Copy link
Member

Thanks for contributing!

@bradzacher
Copy link
Contributor

bradzacher commented Nov 21, 2020

As discussed on the ticket, it seems to be common practice to disable this rule when using typescript-parser

This is because there is an extension version of this rule which adds support for TS features (including fixing this crash): @typescript-eslint/no-useless-constructor.

Base rules that are broken for TS code is our number one FAQ: https://github.com/typescript-eslint/typescript-eslint/blob/master/docs/getting-started/linting/FAQ.md#i-am-using-a-rule-from-eslint-core-and-it-doesnt-work-correctly-with-typescript-code

I'm commenting here as a bit of an FYI for the eslint maintainers - feel free to forward people to our FAQ or to file issues with us.

@mdjermanovic
Copy link
Member

I'm commenting here as a bit of an FYI for the eslint maintainers - feel free to forward people to our FAQ or to file issues with us.

We're always doing that (actually, forwarding people directly to the extension rule if it exists, or to open an issue if it doesn't; thanks for the FAQ link!). This was just a small fix to at least not crash the whole linting.

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators May 18, 2021
@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 May 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3rd party plugin This is an issue related to a 3rd party plugin, config, or parser 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 bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

no-useless-constructor: Cannot read property 'body' of null
4 participants