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

--strict-equality should be more flexible #6607

Closed
ilevkivskyi opened this issue Apr 1, 2019 · 10 comments · Fixed by #6674
Closed

--strict-equality should be more flexible #6607

ilevkivskyi opened this issue Apr 1, 2019 · 10 comments · Fixed by #6674

Comments

@ilevkivskyi
Copy link
Member

Currently, --strict-equality is all or nothing. People typically want it for particular situations like str vs bytes for Python 2 to 3 migration, int vs float for numeric code, etc. It looks like there are at least two options:

  1. Split the flag into several separate flags for most common use cases, similar to how it is done for --disallow-any-xxx flags.
  2. Make it a flag with arguments, similar to how it is done for --always-true/--always-false.

I personally prefer the second option, since it is more flexible. Plus I could imagine a use case with user defined types: one might want to disable comparisons after migration from a legacy API.

@JukkaL
Copy link
Collaborator

JukkaL commented Apr 2, 2019

I think that splitting the option may be a reasonable long-term goal, but I think that it would be too much complexity right now. In my opinion the main benefits for the feature are a) helping with Python 2 to 3 migration and b) catching some silly mistakes such as comparing against a function object (forgetting to call). For these goals, minimizing the false positive rate seems like the main concern, as long as it can still flag things like 'foo' == b'bar' and x.method == 3.

Here are more concrete ideas:

  1. Don't flag int vs float comparison, since apparently people do these a lot and don't consider it a problem. We shouldn't be too opinionated here, I think, since some int vs float comparisons are fine. This can be special cased in the implementation, since this seems such as common thing.
  2. Fix any other false positives (unless they are super obscure) that we come across as bugs. Again I think that it's okay to err on the side of leniency to avoid false positives.

I'd love if we could get the false positive rate low enough to enable this by default. If we can pull that off, the usefulness of the feature would increase dramatically.

@ilevkivskyi
Copy link
Member Author

Here is a possible short term plan we discussed with @JukkaL elsewhere:

  • Fix --strict-equality prohibits a valid check b'abc' in b'cde' #6608.
  • Enable type promotions, so that bytes vs str will be only flagged on Python 3, int vs float will be always safe.
  • Only prohibit the comparisons for types whose __eq__() comes from builtins (don't complain if at least one of the types has a "custom" method).

This will trade some "false positives" for "false negatives", but it is probably OK for now.

@gvanrossum
Copy link
Member

Good plan.

@JukkaL
Copy link
Collaborator

JukkaL commented Apr 3, 2019

Another thing that generates false positives is comparisons between two non-builtin classes. These can be valid due to multiple inheritance (technically there could be multiple inheritance from built-in classes, but that seems rare enough that we can ignore it). Example:

class A: pass
class B: pass
class C(A, B): pass

def f(a: A, b: B) -> bool:
    return a == b  # Should be okay?

c = C()
print(f(c, c))

This causes two false positives in the mypy codebase, so it's not a theoretical concern. This probably mostly happens with ABC-like classes, but some of these might not be tagged as ABCs, so we may have to assume any user-defined class could be used like an ABC.

@ilevkivskyi
Copy link
Member Author

ilevkivskyi commented Apr 3, 2019

These two places in mypy code are actually questionable IMO (because of #5159). I remember I spent some time understanding what is going there while working on aststripnew.py recently.

Also there are several other places where mypy thinks that multiple inheritance doesn't exist, for example isinstance() checks and overloads. I don't think we need to do any special-casing here, currently the same function is_overlapping_types() is called for all these cases.

@JukkaL
Copy link
Collaborator

JukkaL commented Apr 3, 2019

Yeah, the current behavior is pretty reasonable, unless it seems to generate many false positives. The mypy codebase isn't big enough to estimate the false positive rate anyway.

@adamtheturtle
Copy link

I get Non-overlapping equality check (left operand type: "KeysView[str]", right operand type: "Set[str]") when checking a dictionary's keys against a set of strings - this can return True.

@ilevkivskyi
Copy link
Member Author

We can potentially also special-case this, but I think this is lower priority than other cases listed above.

@wittekm
Copy link
Contributor

wittekm commented Apr 12, 2019

per Dropbox Slack, adding this case regarding typing_extensions.Literal:
https://play-mypy.ymyzk.com/?gist=912dd3c477712c0b76a6927cc1ca03a8

@ilevkivskyi
Copy link
Member Author

@wittekm I think this is a separate issue, essentially this depends on the implementation of type narrowing by equality checks, see #5935. Basically, Literal[...].__eq__() should provide a literal context for its argument. I will open a separate issue.

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

Successfully merging a pull request may close this issue.

5 participants