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-unnecessary-condition rule #699

Merged
merged 13 commits into from Sep 12, 2019

Conversation

Retsam
Copy link
Contributor

@Retsam Retsam commented Jul 12, 2019

Adds a no-unnecessary-condition, a rule that uses type information to determine if a condition is necessary. Any condition that's indicated by types to always be truthy or falsy is considered to be unnecessary. Essentially a stronger version of no-constant-condition from core eslint.

For example:

const items = ['foo', 'bar'];
if(items) { } // Error: items is always truthy

The code is largely adapted from strict-boolean-expressions: with tslint, I believe you can get fairly similar behavior to this rule by using their strict-boolean-expressions rule by turning on a lot of options ("allow-null-union", "allow-string", "allow-enum", "allow-number", "allow-boolean-or-undefined"); I was originally going to add some of those options to this codebases version of strict-boolean-expressions, but I believe this just made more sense as its own rule, both in terms of implementation and usage.

Adds a no-unnecessary-condition, a rule that uses type information to determine if a condition is necessary. Any condition that's indicated by types to always be truthy or falsy is considered to be unnecessary. Essentially a stronger version of no-constant-condition from core eslint.

For example:

```ts
const items = ['foo', 'bar'];
if(items) { } // Error: items is always truthy
```
Since no-unnecessary-conditions and strict-boolean-expressions play similar roles but with different levels of strictness, mark them as related to each other in their docs
@Retsam
Copy link
Contributor Author

Retsam commented Sep 4, 2019

Is there anything blocking this from merging? It's had some review and I don't think any action items were raised.

I'd love this to go in so that I can use it, and also because this PR has been passively accumulating maintenance items (prettier upgrade, linter rules change, requiresTypeChecking meta field added) the longer it sits open.

@bradzacher
Copy link
Member

bradzacher commented Sep 5, 2019

It's been sitting at the top of my queue to review.
It's a non-trivial PR, and I haven't had a long enough contiguous block of time to sit down review it, sorry.

I was also spending some time mulling over the question of whether or not this should just exist as part of strict-boolean-expressions.
I'm still not 100%, but I do think that this does work fine as a separate rule, as this lets you check constant conditions.

@bradzacher
Copy link
Member

bradzacher commented Sep 5, 2019

One question @Retsam - do you think it makes sense to add in support for ==/!=/===/!== here - i.e. some rudimentary checking for constant equality checks of non-primitive values like:

enum Foo {
  a = 1,
  b = 2
}

const x: Foo.a = Foo.a;
if (x === Foo.a) {} // lint error

const y = 1;
if (y === 1) {} // lint error

@Retsam
Copy link
Contributor Author

Retsam commented Sep 5, 2019

There's enough overlap in intent and structure with strict-boolean-expressions that I could hypothetically imagine them being combined, but I think the resulting rule would be more confusing than having them separate.

I couldn't summarize the purpose of the combined rule in a sentence, which seems a reasonable heuristic that there's maybe not as much overlap in intent as there may appear on first glance.

Plus the options for strict-boolean-expression don't really make sense with the "unnecessary" idea: there's a PR for allowNullable, and the original TSLint version also has options like "allow-string", "allow-number", "allow-enum", which don't make sense either. If the "necessary" behavior were added as an option to the existing rule, it'd be exclusive to most of the other options. Which could be done, but seems a bit awkward.

On the other hand, the strict-boolean-expressions rule is currently pretty much just a stronger version of this rule (you'd get very little gain from adding this rule on top of that one), so maybe that's an argument towards consolidation.

@Retsam
Copy link
Contributor Author

Retsam commented Sep 5, 2019

I'll take a look at the equality checking idea. It definitely seems like it's in scope for this rule's purpose.


And thanks for taking another look at this; I know it's volunteer time, and it's appreciated.

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.

Code overall LGTM - thanks for your contribution.
A few minor comments.

LMK your thoughts on the equality piece when you've had a chance to think about it :)

If both sides of a boolean expression are literals, the condition is unnecessary
@Retsam
Copy link
Contributor Author

Retsam commented Sep 6, 2019

I added support for the equality operators (and also the relational operators): I checked if both sides of the operator are literal values, if they are then the expression is unnecessary.

The only other case of an unnecessary equality operator I could think of is comparing two entirely disjoint types, e.g. a "foo" | "bar" variable being compared against a "baz" variable: but the TS compiler already errors in that case.


I gave this case a generic error message "both sides of the expression are literal values" - in theory we could use the same existing error messages ("always truthy" or "always falsy"), but the code to determine which to use in each case was annoyingly verbose and a bit type unfriendly. I can still go that route if you think it's worth doing.


I'm also fine punting the equality case to a different PR if you'd prefer to review it separately. (Easy to drop that commit from this PR and move it)

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.

I'm also fine punting the equality case to a different PR if you'd prefer to review it separately.

No need - github lets you compare across commits within a PR, so it's super simple to review the new changes.

This is why I always tell people off for force pushing to PR branches, because github doesn't track changes across a force push.


The new changes LGTM.

A few very minor nits, and then I'm happy!

Thanks for working on this.

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.

(was supposed to RC woops)

Co-Authored-By: Brad Zacher <brad.zacher@gmail.com>
@bradzacher bradzacher added the 1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready label Sep 7, 2019
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 doing this.

Copy link
Member

@JamesHenry JamesHenry left a comment

Choose a reason for hiding this comment

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

Thanks!

@JamesHenry JamesHenry removed the 1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready label Sep 12, 2019
@codecov
Copy link

codecov bot commented Sep 12, 2019

Codecov Report

Merging #699 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #699      +/-   ##
==========================================
+ Coverage   94.08%   94.13%   +0.04%     
==========================================
  Files         114      115       +1     
  Lines        4956     4996      +40     
  Branches     1382     1393      +11     
==========================================
+ Hits         4663     4703      +40     
  Misses        166      166              
  Partials      127      127
Impacted Files Coverage Δ
...slint-plugin/src/rules/no-unnecessary-condition.ts 100% <100%> (ø)
packages/eslint-plugin/src/rules/index.ts 100% <100%> (ø) ⬆️

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.

None yet

4 participants