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

feat(eslint-plugin): add no-non-null-asserted-nullish-coalescing rule #3349

Conversation

sonallux
Copy link
Contributor

@sonallux sonallux commented May 4, 2021

Adds a no-non-null-asserted-nullish-coalescing rule that disallows using the non-null assertion on the left operand of the nullish coalescing operator. The non-null assertion is not redundant here because the nullish coalescing operator is explicitly handling null and undefined.

Incorrect code for this rule:

foo! ?? bar

fixes #2853

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @sonallux!

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.

@bradzacher bradzacher added the enhancement: new plugin rule New rule request for eslint-plugin label May 4, 2021
@sonallux sonallux force-pushed the 2853-non-null-asserted-nullish-coalescing branch from 6e00a08 to c5aab46 Compare May 4, 2021 19:34
@codecov
Copy link

codecov bot commented May 4, 2021

Codecov Report

Merging #3349 (db9d5b9) into master (0bb96ca) will decrease coverage by 0.00%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master    #3349      +/-   ##
==========================================
- Coverage   93.53%   93.52%   -0.01%     
==========================================
  Files         149      150       +1     
  Lines        8043     8067      +24     
  Branches     2550     2556       +6     
==========================================
+ Hits         7523     7545      +22     
- Misses        164      165       +1     
- Partials      356      357       +1     
Flag Coverage Δ
unittest 93.52% <91.66%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
packages/eslint-plugin/src/configs/all.ts 100.00% <ø> (ø)
...c/rules/no-non-null-asserted-nullish-coalescing.ts 91.66% <91.66%> (ø)

Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

Everything looks great so far! But there's one case we probably need to handle the case of a variable being used before it's assigned.

let x: string;
x! ?? '';

Reporting here is bad because there's no way for the user to satisfy it. Which is annoying and reduces user trust.

I think that we could handle this using scope analysis.

So by getting the variable declaration you can inspect its usages to see if it has any writes to it, and if it doesn't then it's a variable that is used before it's assigned.

You can also check the declaration for the weird "trust me it's assigned" syntax: const x!: number

Let me know if you want any pointers on how to use the scope analyser.
You can search this codebase for context.getScope() for a few examples.

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label May 15, 2021
@sonallux
Copy link
Contributor Author

Everything looks great so far! But there's one case we probably need to handle the case of a variable being used before it's assigned.

let x: string;
x! ?? '';

@bradzacher I have added support for handling the use-before-assigned case. I hope I did not forget anything.

@sonallux sonallux force-pushed the 2853-non-null-asserted-nullish-coalescing branch from 4bf9da1 to 9379caa Compare May 16, 2021 17:53
@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Jul 6, 2021
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

Tests look good. Implementation is almost correct.
Thanks for your work here!

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Jul 31, 2021
@sonallux
Copy link
Contributor Author

sonallux commented Aug 3, 2021

Tests look good. Implementation is almost correct.
Thanks for your work here!

@bradzacher Thanks for the feedback. I have already fixed all your comments.

@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Aug 3, 2021
bradzacher
bradzacher previously approved these changes Sep 20, 2021
Copy link
Member

@bradzacher bradzacher 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 for this!

# Conflicts:
#	packages/eslint-plugin/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: new plugin rule New rule request for eslint-plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[new rule] Disallow !?? (Non-null + nullish)
2 participants