-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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] Use Unknown | T_inferred
for undeclared public symbols
#15674
base: main
Are you sure you want to change the base?
Changes from all commits
f7f3d92
9bf84e5
7a035b7
5bf3c9b
49bcf53
f29add5
36b3af6
f4ca4b8
a1c5647
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,7 +79,7 @@ def _(flag: bool): | |
# that `Foo.__iadd__` may be unbound as additional context. | ||
f += "Hello, world!" | ||
|
||
reveal_type(f) # revealed: int | Unknown | ||
reveal_type(f) # revealed: Unknown | int | ||
``` | ||
|
||
## Partially bound with `__add__` | ||
|
@@ -96,7 +96,7 @@ def _(flag: bool): | |
f = Foo() | ||
f += "Hello, world!" | ||
|
||
reveal_type(f) # revealed: int | str | ||
reveal_type(f) # revealed: Unknown | int | str | ||
``` | ||
|
||
## Partially bound target union | ||
|
@@ -116,7 +116,7 @@ def _(flag1: bool, flag2: bool): | |
f = 42.0 | ||
f += 12 | ||
|
||
reveal_type(f) # revealed: int | str | float | ||
reveal_type(f) # revealed: Unknown | int | str | float | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question; why does |
||
``` | ||
|
||
## Target union | ||
|
@@ -160,5 +160,5 @@ def f(flag: bool, flag2: bool): | |
f = Bar() | ||
f += 12 | ||
|
||
reveal_type(f) # revealed: int | str | float | ||
reveal_type(f) # revealed: Unknown | int | str | float | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And same question here. |
||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -262,7 +262,8 @@ class A: | |
class B: | ||
__add__ = A() | ||
|
||
reveal_type(B() + B()) # revealed: int | ||
# TODO: this could be `int` if we declare `B.__add__` using a `Callable` type | ||
reveal_type(B() + B()) # revealed: Unknown | int | ||
Comment on lines
+265
to
+266
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is one example of a particularly annoying case that also comes up with The symbol There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not too worried about this, because almost always these will be normal methods defined within the class (which is a declaration), not assigned like this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it would be nice if there was an However, realistically: it's very unusual to use non-function instances as methods like this. This kind of thing is quite common: class Foo:
def __or__(self, other) -> int:
return 42
__ror__ = __or__ But I think that will still be fine with this change, since functions have the same declared type as their inferred type? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I think we'd have a problem with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. It would be the exact same case as with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I see, thanks. This seems bad :( I just don't know how we'd explain this to users |
||
``` | ||
|
||
## Integration test: numbers from typeshed | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,18 +5,23 @@ that is, a use of a symbol from another scope. If a symbol has a declared type i | |
(e.g. `int`), we use that as the symbol's "public type" (the type of the symbol from the perspective | ||
of other scopes) even if there is a more precise local inferred type for the symbol (`Literal[1]`). | ||
|
||
If a symbol has no declared type, we use the union of `Unknown` with the inferred type as the public | ||
type. If there is no declaration, then the symbol can be reassigned to any type from another scope; | ||
the union with `Unknown` reflects that its type must at least be as large as the type of the | ||
assigned value, but could be arbitrarily larger. | ||
|
||
We test the whole matrix of possible boundness and declaredness states. The current behavior is | ||
summarized in the following table, while the tests below demonstrate each case. Note that some of | ||
this behavior is questionable and might change in the future. See the TODOs in `symbol_by_id` | ||
(`types.rs`) and [this issue](https://github.com/astral-sh/ruff/issues/14297) for more information. | ||
In particular, we should raise errors in the "possibly-undeclared-and-unbound" as well as the | ||
"undeclared-and-possibly-unbound" cases (marked with a "?"). | ||
|
||
| **Public type** | declared | possibly-undeclared | undeclared | | ||
| ---------------- | ------------ | -------------------------- | ------------ | | ||
| bound | `T_declared` | `T_declared \| T_inferred` | `T_inferred` | | ||
| possibly-unbound | `T_declared` | `T_declared \| T_inferred` | `T_inferred` | | ||
| unbound | `T_declared` | `T_declared` | `Unknown` | | ||
| **Public type** | declared | possibly-undeclared | undeclared | | ||
| ---------------- | ------------ | ------------------------------------- | ----------------------- | | ||
| bound | `T_declared` | `Unknown \| T_declared \| T_inferred` | `Unknown \| T_inferred` | | ||
| possibly-unbound | `T_declared` | `Unknown \| T_declared \| T_inferred` | `Unknown \| T_inferred` | | ||
| unbound | `T_declared` | `Unknown \| T_declared` | `Unknown` | | ||
|
||
| **Diagnostic** | declared | possibly-undeclared | undeclared | | ||
| ---------------- | -------- | ------------------------- | ------------------- | | ||
|
@@ -106,8 +111,8 @@ if flag(): | |
```py | ||
from mod import x, y | ||
|
||
reveal_type(x) # revealed: Literal[1] | Any | ||
reveal_type(y) # revealed: Literal[2] | Unknown | ||
reveal_type(x) # revealed: Unknown | Literal[1] | Any | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably cancel one of the dynamic types here? We might have a ticket for that. I'll note it down as a follow up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we have a TODO in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that we should do that, but it will also make this test less clearly testing what it is supposed to test. The reason we use But we can discuss how to handle this in the follow up PR. |
||
reveal_type(y) # revealed: Unknown | Literal[2] | ||
``` | ||
|
||
### Possibly undeclared and possibly unbound | ||
|
@@ -132,8 +137,8 @@ else: | |
# error: [possibly-unbound-import] | ||
from mod import x, y | ||
|
||
reveal_type(x) # revealed: Literal[1] | Any | ||
reveal_type(y) # revealed: Literal[2] | str | ||
reveal_type(x) # revealed: Unknown | Literal[1] | Any | ||
reveal_type(y) # revealed: Unknown | Literal[2] | str | ||
``` | ||
|
||
### Possibly undeclared and unbound | ||
|
@@ -153,23 +158,21 @@ if flag(): | |
# on top of this document. | ||
from mod import x | ||
|
||
reveal_type(x) # revealed: int | ||
reveal_type(x) # revealed: Unknown | int | ||
``` | ||
|
||
## Undeclared | ||
|
||
### Undeclared but bound | ||
|
||
We use the inferred type as the public type, if a symbol has no declared type. | ||
|
||
```py path=mod.py | ||
x = 1 | ||
``` | ||
|
||
```py | ||
from mod import x | ||
|
||
reveal_type(x) # revealed: Literal[1] | ||
reveal_type(x) # revealed: Unknown | Literal[1] | ||
``` | ||
|
||
### Undeclared and possibly unbound | ||
|
@@ -189,7 +192,7 @@ if flag: | |
# on top of this document. | ||
from mod import x | ||
|
||
reveal_type(x) # revealed: Literal[1] | ||
reveal_type(x) # revealed: Unknown | Literal[1] | ||
``` | ||
|
||
### Undeclared and unbound | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,7 +29,7 @@ def _(flag: bool): | |
|
||
a = PossiblyNotCallable() | ||
result = a() # error: "Object of type `PossiblyNotCallable` is not callable (possibly unbound `__call__` method)" | ||
reveal_type(result) # revealed: int | ||
reveal_type(result) # revealed: Unknown | int | ||
``` | ||
|
||
## Possibly unbound callable | ||
|
@@ -52,7 +52,7 @@ class NonCallable: | |
__call__ = 1 | ||
|
||
a = NonCallable() | ||
# error: "Object of type `NonCallable` is not callable" | ||
# error: "Object of type `Unknown | Literal[1]` is not callable (due to union element `Literal[1]`)" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another instance of the callable problem. Notice how the usefulness of the error message degrades. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is really begging for nested diagnostics (so you get both "Object of type We could easily adjust the handling so you still get "Object of type Even without nested diagnostics we could still provide all of the information here in a new error, it just requires more and more bespoke error-cases handling in the cc @BurntSushi @MichaReiser re diagnostics considerations |
||
reveal_type(a()) # revealed: Unknown | ||
``` | ||
|
||
|
@@ -67,7 +67,7 @@ def _(flag: bool): | |
def __call__(self) -> int: ... | ||
|
||
a = NonCallable() | ||
# error: "Object of type `Literal[1] | Literal[__call__]` is not callable (due to union element `Literal[1]`)" | ||
# error: "Object of type `Unknown | Literal[1] | Literal[__call__]` is not callable (due to union element `Literal[1]`)" | ||
reveal_type(a()) # revealed: Unknown | int | ||
``` | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,7 +43,8 @@ class IntIterable: | |
def __iter__(self) -> IntIterator: | ||
return IntIterator() | ||
|
||
# revealed: tuple[int, int] | ||
# TODO: This could be a `tuple[int, int]` if we model that `y` can not be modified in the outer comprehension scope | ||
# revealed: tuple[int, Unknown | int] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. every comprehension has its own scope, so we also look up There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this is something @AlexWaygood wanted to look into, if I'm not mistaken (after outcome) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, for nested scopes that are known to execute eagerly (list/set/dict comprehensions, class bodies), and for ones that probably execute eagerly (generator expressions), we should model the name lookup as a "use" in the outer scope, at that point in control flow of the outer scope, rather than as a public type lookup. Alex has WIP on this, and it should fix this TODO and the one below. |
||
[[reveal_type((x, y)) for x in IntIterable()] for y in IntIterable()] | ||
``` | ||
|
||
|
@@ -66,7 +67,8 @@ class IterableOfIterables: | |
def __iter__(self) -> IteratorOfIterables: | ||
return IteratorOfIterables() | ||
|
||
# revealed: tuple[int, IntIterable] | ||
# TODO: This could be a `tuple[int, int]` (see above) | ||
# revealed: tuple[int, Unknown | IntIterable] | ||
[[reveal_type((x, y)) for x in y] for y in IterableOfIterables()] | ||
``` | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -172,10 +172,10 @@ class IntUnion: | |
def __len__(self) -> Literal[SomeEnum.INT, SomeEnum.INT_2]: ... | ||
|
||
reveal_type(len(Auto())) # revealed: int | ||
reveal_type(len(Int())) # revealed: Literal[2] | ||
reveal_type(len(Int())) # revealed: int | ||
reveal_type(len(Str())) # revealed: int | ||
reveal_type(len(Tuple())) # revealed: int | ||
reveal_type(len(IntUnion())) # revealed: Literal[2, 32] | ||
reveal_type(len(IntUnion())) # revealed: int | ||
Comment on lines
174
to
+178
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wow, um... it's not your fault, but I think this test should definitely have some comments next to it saying how we don't really support enums at all yet 😄 I think I can see why the result changes here, but it seems incorrect that we allow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I also seem to remember that we said in the original PR that those test cases are not particularly important, so I didn't bother to fix them / add a TODO. Let me know if you think otherwise. |
||
``` | ||
|
||
### Negative integers | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -99,9 +99,9 @@ def _(x: str | int): | |
class A: ... | ||
class B: ... | ||
|
||
alias_for_type = type | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can not annotate this alias as
|
||
|
||
def _(x: A | B): | ||
alias_for_type = type | ||
|
||
if alias_for_type(x) is A: | ||
reveal_type(x) # revealed: A | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand where the new
Unknown
is coming from here? We are revealing the type off
locally within the scope, so it shouldn't be due to modifiability off
. And both__add__
and__iadd__
are declared in the body ofFoo
(because function definition statements are declarations), so I don't think either of those should be unioned withUnknown
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__iadd__
is possibly undeclared.I'm not 100% clear on the semantics of
__add__
and__iadd__
, so I suspect that you think that this possibly-undeclaredness should be absorbed by the definite-declaredness of__add__
?I'll look into that tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I said off the top of my head that we should add
Unknown
to the union for possibly-undeclared, because the "declared type" in the undeclared path isUnknown
. But now I'm wondering if that's just wrong. An external assignment that violates the possibly-declared type would effectively be creating a conflicting-declarations situation, and we currently error on conflicting declarations, so it seems like we should also error on that assignment. Which we would do, if we didn't union withUnknown
for possibly-undeclared.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me. To make it more concrete, my understanding is that we would always want to show an error in the last line here, because it is possible that
flag
isTrue
, and in this case, we would violate the declaration.we would always want to show an error in the last line.
I'll revert f4ca4b8 and add a new test.