Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Add strict-comparisons Rule #4519

Merged

Conversation

AndreasGassmann
Copy link
Contributor

@AndreasGassmann AndreasGassmann commented Feb 15, 2019

PR checklist

Overview of change:

This rule disallows the comparison of objects.

>, >=, <, <= are only allowed if the operands are of type number.
=== and !== are only allowed for operands of type number, string, boolean and enum or if either of the operands is null or undefined.

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

This is my first tslint rule and in general the first time I took a closer look at the typescript internals.

I'm not sure if all the type checks are bullet proof. I'm pretty sure there's a better way to do those which I haven't figured out yet.

CHANGELOG.md entry:

[new-rule] strict-comparison

@AndreasGassmann
Copy link
Contributor Author

Looks like there are some cases that I didn't think about. Some of the things I saw:

  • Numeric enums should be allowed with >, <, <= and >=. (How can I find out if it's a numeric enum or not?)
  • Some people do use >, <, <= and >= with strings. Should I add an option to allow that?

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Posted a few comments on the rule name and that large conditional.

Love the test cases!

src/rules/noObjectComparisonRule.ts Outdated Show resolved Hide resolved
src/rules/noObjectComparisonRule.ts Outdated Show resolved Hide resolved
src/rules/noObjectComparisonRule.ts Outdated Show resolved Hide resolved
@AndreasGassmann AndreasGassmann changed the title Add no-object-comparison Rule Add object-comparison Rule Feb 17, 2019
@AndreasGassmann
Copy link
Contributor Author

AndreasGassmann commented Feb 18, 2019

I see a couple of things that are still open:

  • Sometimes, enum comparisons seem incorrect (eg. looking at the failing circleci: lint step here). I was not able to reproduce this, so I'm not sure what's going on.

  • < and > should be allowed between Numeric Enums, but not between String Enums (without the allow-string-comparison option). However, I'm not sure how to do that because it doesn't seem like those are different types in typescript.

  • Rename allow-equal and allow-string-comparison because it's not clear what they do right now.
    allow-equal could be named allow-object-equal-comparison
    allow-string-comparison could be named allow-string-order-comparison (I'm not sure how to call the "ordering" operators like >, <, >=, <=).

  • Corner cases including any undefined and null have to be handled.

@JoshuaKGoldberg JoshuaKGoldberg merged commit 5d135e8 into palantir:master Jun 16, 2019
@JoshuaKGoldberg
Copy link
Contributor

All right, this is in! Thanks so much @AndreasGassmann for getting started on the rule implementation and working earlier this year on the initial rounds of feedback. strict-comparisons will be available in the next TSLint version.

@rolftimmermans
Copy link

rolftimmermans commented Jul 15, 2019

Feedback from a user who likes to use "extends": ["tslint:all"] to test new rules: I don't think this rule is correct in its current form.

Example code :

function foo(s: string | undefined) {
  if (s === undefined) return "none"
  return s
}

foo(undefined)

Lint error with this rule enabled:
foo.ts[2, 7]: Cannot use '===' comparator for type 'undefined'.

To me there seems absolutely nothing wrong with testing a variable of type string | undefined for equality with undefined.

@JoshuaKGoldberg
Copy link
Contributor

Feedback from a user who likes to use "extends": ["tslint:all"]

Love it! 💖

Could you please file a new issue for feedback? Separate issues and PRs for different topics are easier to track and search for.

@rolftimmermans
Copy link

rolftimmermans commented Jul 15, 2019

Could you please file a new issue for feedback? Separate issues and PRs for different topics are easier to track and search for.

Absolutely! Searching through the issues my experience seems similar or identical to #4774, so I'll leave it at that. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule suggestion: Ban comparisons of non-primitives
5 participants