Skip to content

[umbrella] Pyrefly should probably respect annotations better #2227

@stroxler

Description

@stroxler

Describe the Bug

I noticed recently that Pyrefly is a bit more clever than I think it should be about using inferred types. We implement assignment narrowing, so that if I have

x: object
x = 5

we'll infer the type of x to be Literal[5]. This is probably fine, since Literal[5] <: int where in this case by <: I mean true subtyping (not just assignability).

When we do this, we do at least try to check assignability, but that's actually not acceptable because it causes type erasure in cases where the user is probably explicitly trying to introduce a typed boundary. For example, consider this code:

from typing import *
def f() -> Any: ...
x: int
x = f()
reveal_type(x)  # reveals Any

We won't do this in the special case of an annotated assignment, which is (a) a little surprising and inconsistent but also (b) isn't good enough in some cases; my trivial example could have been written x: int = f() but in a lot of code there are multiple assignments and many linters actually dislike duplicated annotations. I believe we absolutely should not be overriding a declared type with a gradual type like this.

On top of that there's a separate collection of bugs where we don't do any checking at all. For example:

from typing import *
def f() -> str: ...
x: int
x = f()  # Errors, correctly
reveal_type(x)  # Reveals int, which seems like the right behavior and is probably intended
x, y = f(), f()  # Errors again, correctly
reveal_type(x)  # oops, now it's a `str`

I find it impossible to believe anyone intended this, I think the logic we use here is probably just so poorly organized (probably just hacked into each operation independently) so that most edge cases wind up actually diverging from the happy case. Part of the problem, from a skim, seems to be that in some cases expr_infer_with_hint might wind up throwing errors using the hint but then returning the actual inferred expr type anyway. That might be the right behavior in some cases, but for binding a value to an annotated name it is never the right thing, we should throw an error and then keep the annotated type for downstream code.

I marked this umbrella because I think there is a collection of mostly unrelated bugs and gaps in the logic that probably have independent fixes, and part of the work is to just figure them out. A particularly egregious one for any async codebase is that we pretty much always override the annotated type when there's a type error in an awaited assignment.

Sandbox Link

No response

(Only applicable for extension issues) IDE Information

No response

Metadata

Metadata

Assignees

Labels

epicAn issue that is an umbrella for work tracked in other issuestypechecking

Type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions