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

Allow narrowing enum values using == #11521

Merged
merged 1 commit into from
Apr 24, 2023
Merged

Conversation

hauntsaninja
Copy link
Collaborator

Resolves #10915, resolves #9786

See the discussion in #10915. I'm sympathetic to the difference between
identity and equality here being surprising and that mypy doesn't
usually make concessions to mutability when type checking.

The old test cases are pretty explicit about their intentions and are
worth reading. Curious to see what people (and mypy-primer) have to say
about this.

Resolves python#10915, resolves python#9786

See the discussion in python#10915. I'm sympathetic to the difference between
identity and equality here being surprising and that mypy doesn't
usually make concessions to mutability when type checking.

The old test cases are pretty explicit about their intentions and are
worth reading. Curious to see what people (and mypy-primer) have to say
about this.
@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

porcupine (https://github.com/Akuli/porcupine.git)
+ porcupine/pluginloader.py:350: error: Statement is unreachable  [unreachable]

@JukkaL
Copy link
Collaborator

JukkaL commented Nov 11, 2021

My gut feeling is that == and is should be treated the same. Treating them differently sounds confusing.

@Michael0x2a
Copy link
Collaborator

Michael0x2a commented Nov 11, 2021

IIRC, treating == and is differently was intended to be a workaround for tests that mutate state and repeatedly do something like assert wrapper.state == StateEnum.foo then later assert wrapper.state == StateEnum.bar.

That latter assert would end up narrowing away wrapper.state to nothing, causing all subsequent code to silently become unreachable unless you opt-in to using the --warn-unreachable flag.

In retrospect, I'm wondering if a better solution might have been to instead overhaul our heuristics for when narrowing happens in general. At the moment, assert unconditionally narrows. But maybe instead it might be better to have it only narrow types and refrain from narrowing literals?

I think in practice the only time you'd want to narrow literals is in if/else statements, so sticking to just narrowing there might be good enough.

Or maybe alternatively, allow literal-type narrowing to sometimes change the type instead of narrowing it?

Higher-level, the problem is that unconditional narrowing combined with mutable state is fundamentally unsound: you can run into the exact same problem even with isinstance(...). But people normally don't change the types of their fields in the same way they do literals, so our heuristics end up being correct most of the time.

Unfortunately, this "always narrow" heuristic doesn't work as well for literal types, which is why I ended up adding a bunch of misc ad-hoc heuristics such as "narrow enums only when using 'is'" to try and patch things up.

In any case, I do agree the heuristics I ended up picking out are unintuitive and probably ought to be changed.

But IMO we should probably design and implement better heuristics first before landing this PR to avoid causing too large of a drop in type-checking coverage for users.

@hauntsaninja
Copy link
Collaborator Author

This keeps coming up in issues, so I'm going to merge this. The status quo is unintuitive and users can still narrow to the bottom type using is, so it's not like we're stopping them from doing it. There is some risk that more code will go effectively unchecked, but if the most common use of this pattern is in tests, it's not the worst thing in the world.

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