Skip to content

Add flag to prohibit equality checks between non-overlapping checks #6370

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

Merged
merged 13 commits into from
Feb 15, 2019

Conversation

ilevkivskyi
Copy link
Member

@ilevkivskyi ilevkivskyi commented Feb 10, 2019

Fixes #1271

The new per-file flag --strict-equality disables equality, identity, and container checks between non-overlapping types. In general the implementation is straightforward. Here are some corner cases I have found:

  • Any should be always safe.
  • Type promotions should be ignored, b'abc == 'abc' should be an error.
  • Checks like if x is not None: ..., are special cased to not be errors if x has non-optional type.
  • Optional[str] and Optional[bytes] should be considered non-overlapping for the purpose of this flag.
  • For structural containers and custom __eq__() (i.e. incompatible with object.__eq__()) I suppress the non-overlapping types error, if there is already an error originating from the method call check.

Note that I updated typing-full.pyi so that Sequence inherits from Container (this is needed by some added tests, and also matches real stubs). This however caused necessary changes in a bunch of builtins fixtures.

@msullivan
Copy link
Collaborator

Interestingly, this flag cause couple dozen errors in mypy itself, vast majority or errors are because of

obj: Instance
if obj is not None:
    ...

In some sense that is a good thing, right, since we are doing a bad thing in those places? On the other hand, there are a lot of them, so...

Copy link
Collaborator

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

This is really great! There are a couple questions worth thinking about though:

  • I think we probably do want to allow promotions, since disallowing int/float comparisons seems a little aggressive.
  • This disallows things like assert foo is not None which, while nominally ruled out by the type system, might be worth cutting some slack to. I dunno if there is a reasonable way to suppress it in asserts? Maybe the right thing is to allow spurious none checks, though that's kind of a bummer.

@ilevkivskyi
Copy link
Member Author

This disallows things like assert foo is not None

OK, also as discussed of tracker, let's start with allowing this asserts (otherwise even mypy codebase wouldn't be clean with this flag).

@ilevkivskyi
Copy link
Member Author

@msullivan
Thanks for review! This is ready for another round.

Copy link
Collaborator

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

One request for some restructuring in the new Type code, but otherwise this is great

@ilevkivskyi ilevkivskyi merged commit 3b511bd into python:master Feb 15, 2019
@ilevkivskyi ilevkivskyi deleted the strict-equality branch February 15, 2019 00:28
@JukkaL
Copy link
Collaborator

JukkaL commented Feb 15, 2019

@ilevkivskyi I think that int/float comparison should be allowed, since they are safe and disallowing them may make this feature much less useful for code that uses lots of floats. Can you create a follow-up issue about this? It may be fine to reject str vs. unicode comparisons, but even there it's a bit questionable, since str and unicode can be freely mixed in other contexts due to the promotion. Why do we disallow this only for this particular operation?

@ilevkivskyi
Copy link
Member Author

It may be fine to reject str vs. unicode comparisons, but even there it's a bit questionable, since str and unicode can be freely mixed in other contexts due to the promotion. Why do we disallow this only for this particular operation?

This request came many times from internal users, since such comparisons caused actual troubles. One of the problems is that b'abc' == 'abc' is True in Python 2, but False in Python 3.

I think that int/float comparison should be allowed, since they are safe and disallowing them may make this feature much less useful for code that uses lots of floats.

As someone who used floats a lot, I think comparing floats with == is a bad idea, because of traps like (1/3)**10 * 3**10 == 1 or (2**0.1)**10 == 2, etc. They should be compared with something like math.isclose(), numpy.allclose() or similar.

As a bottom line, I still think prohibiting promotions with this flag is a good idea. Potentially, we can add a note why this is unsafe.

@JukkaL
Copy link
Collaborator

JukkaL commented Feb 15, 2019

Thanks for the detailed explanation -- you convinced me. This is a great feature as it is now and will help catch many bugs.

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

Successfully merging this pull request may close these issues.

Stricter equality checks?
3 participants