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

[red-knot] declarations in try blocks #13966

Closed
carljm opened this issue Oct 28, 2024 · 0 comments · Fixed by #14958
Closed

[red-knot] declarations in try blocks #13966

carljm opened this issue Oct 28, 2024 · 0 comments · Fixed by #14958
Assignees
Labels
red-knot Multi-file analysis & type inference
Milestone

Comments

@carljm
Copy link
Contributor

carljm commented Oct 28, 2024

Type declarations via variable annotations in red-knot are control-flow sensitive. That is, if you have

if flag:
    x: str = "foo"
else:
    x: int = 1

We will allow that code. But we currently error if you try to assign to a variable at a point in control flow that is reachable by multiple conflicting declarations:

if flag:
    x: str = "foo"
else:
    x: int = 1

# Whether this assignment is valid or not is somewhat ambiguous,
# so we currently error on it with "conflicting declared types, int vs str".
# The error can be fixed by making this `x: int = 3` instead, which
# explicitly overrides the conflicting prior declarations with a new one.
# We could also just choose to stop emitting that error entirely, and
# allow assignment of either ints or strings here (declared type
# implicitly becomes `int | str` at the control flow join point.)
x = 3

We also consider "no declaration" (which is implicitly a declaration of Unknown, allowing all assignments to that variable) as a conflict with "some declaration", so this code is also currently an error:

if flag:
    x: int = 1
x = 2  # conflicting declarations, `int` vs `Unknown`

Other options here would include just letting the declared type implicitly be int | Unknown after the end of the if flag: block, or ignoring undeclared paths and just having the post-if declared type be int.

The current behavior is particularly problematic with a try block. In this code:

try:
    x: int = call_that_may_raise()
except:
    x = 2

Our understanding of control flow in try blocks accounts for the fact that we could enter the except block without having run some or any of the code in the try block (in case of an exception.) Since we reuse that same understanding of control flow for deciding where declarations reach, we end up seeing "conflicting declarations int vs Unknown" at the x = 2 assignment, because we see that control flow can reach that point with or without having fully executed the line x: int = call_that_may_raise().

This doesn't make intuitive sense, because type declarations are a static analysis concept, not a runtime concept, and control-flow jumps due to exceptions are implicit runtime behavior, not obvious in the code structure like other control-flow constructs. It would be better in this case if we considered the x: int declaration to be unconditional.

If we ignored undeclared paths (as discussed above), that would make the undeclared-path part of this problem disappear.

You could still get "conflicting declarations" with code like this:

try:
    x: int = 1
    x: str = "foo"
except:
    x = "bar"

Where (due to exception control flow) we would see both the x: int and the x: str declarations as "live" at x = "bar". But this seems much less likely in real code.

Another option here is to dis-regard exception control flow entirely when it comes to type declarations. (We would still consider it in type inference, just not for type declarations.) So in the previous two examples, when it comes to deciding what the live declarations of x are, we would ignore the possibility of jumping directly into except from partway through the try, and we would consider all declarations in the try block as unconditional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants