[red-knot] Trust module-level undeclared symbols in stubs #17577
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
sharkdp
force-pushed
the
david/trust-undeclared-symbols-in-stubs
branch
from
234bc3a to
b68e294
Compare
sharkdp
force-pushed
the
david/trust-undeclared-symbols-in-stubs
branch
from
b68e294 to
bf60832
Compare
sharkdp
changed the title
AlexWaygood
approved these changes
Apr 23, 2025
There was a problem hiding this comment.
Yeah, this makes sense to me.
I do still think a lot of these assignments should probably be annotated with Final in typeshed anyway! This would benefit both us and other type checkers.
But your ecosystem analysis indicates that this is also a broader problem than just typeshed: attrs also appears to use bare assignments without declarations in its stub files, etc.
| @@ -593,8 +593,18 @@ fn symbol_by_id<'db>( | |||
| "__slots__" | "TYPE_CHECKING" | |||
| ); | |||
|
|
|||
| widen_type_for_undeclared_public_symbol(db, inferred, is_considered_non_modifiable) | |||
| .into() | |||
| if scope.is_module_scope(db) && scope.file(db).is_stub(db.upcast()) { | |||
There was a problem hiding this comment.
why only at the module level? Typeshed does things like this as well, and I think there's an argument to be made that the public type for DeprecatedNameForThing should also not be unioned with Unknown here:
class Foo:
THING: Final = 42
DeprecatedNameForThing = THINGit feels consistent with the idea behind this PR.
There was a problem hiding this comment.
I didn't have this restriction initially, but then added it because Carl suggested this as a rule here: #17032 (comment). What was your reasoning @carljm?
There was a problem hiding this comment.
I'm going to do the following: I'm going to merge this as-is, and then re-open another PR with that restriction lifted. This should allow us to see the impact in a better way.
There was a problem hiding this comment.
I do think the same rationale applies; I don't think I carefully thought through the class-scope case as even a thing that would happen in stub files.
sharkdp
force-pushed
the
david/trust-undeclared-symbols-in-stubs
branch
from
bf60832 to
863a04a
Compare
dcreager
added a commit
that referenced
this pull request
Apr 23, 2025
…tructor * origin/main: [red-knot] Trust module-level undeclared symbols in stubs (#17577) [`airflow`] Apply auto fixes to cases where the names have changed in Airflow 3 (`AIR301`) (#17355) [`pycodestyle`] Auto-fix redundant boolean comparison (`E712`) (#17090) [red-knot] Detect semantic syntax errors (#17463) Fix stale diagnostics in Ruff playground (#17583) [red-knot] Early return from `project.is_file_open` for vendored files (#17580)
sharkdp
added a commit
that referenced
this pull request
Apr 23, 2025
## Summary *Generally* trust undeclared symbols in stubs, not just at the module level. Follow-up on the discussion [here](#17577 (comment)). ## Test Plan New Markdown test.
dcreager
added a commit
that referenced
this pull request
Apr 23, 2025
…var-instance * dcreager/generic-constructor: Revert FunctionLiteral type [red-knot] Trust module-level undeclared symbols in stubs (#17577) [`airflow`] Apply auto fixes to cases where the names have changed in Airflow 3 (`AIR301`) (#17355) [`pycodestyle`] Auto-fix redundant boolean comparison (`E712`) (#17090) Clean this up a bit clippy [red-knot] Detect semantic syntax errors (#17463) Fix stale diagnostics in Ruff playground (#17583) [red-knot] Early return from `project.is_file_open` for vendored files (#17580)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Many symbols in typeshed are defined without being declared. For example:
Here, we introduce a change that skips widening the public type of these symbols (by unioning with
Unknown).fixes #17032
Ecosystem analysis
This is difficult to analyze in detail, but I went over most changes and it looks very favorable to me overall. The diff on the overall numbers is:
Removed false positives
invalid-baseexamples:invalid-exception-caughtexamples:unresolved-referenceexampleshttps://github.com/canonical/cloud-init/blob/7a0265d36e01e649f72005548f17dca9ac0150ad/cloudinit/handlers/jinja_template.py#L120-L123 (we now understand the
isinstancenarrowing)- error[lint:unresolved-attribute] /tmp/mypy_primer/projects/cloud-init/cloudinit/handlers/jinja_template.py:123:16: Type `Exception` has no attribute `errno`unknown-argumentexampleshttps://github.com/hauntsaninja/boostedblob/blob/master/boostedblob/request.py#L53
- error[lint:unknown-argument] /tmp/mypy_primer/projects/boostedblob/boostedblob/request.py:53:17: Argument `connect` does not match any known parameter of bound method `__init__`unknown-argumentThere are a lot of
__init__-related changes because we now understand@attr.sas a@dataclass_transformannotated symbol. For example:- error[lint:unknown-argument] /tmp/mypy_primer/projects/attrs/tests/test_hooks.py:72:18: Argument `x` does not match any known parameter of bound method `__init__`New false positives
This can happen if a symbol that previously was inferred as
X | Unknownwas assigned-to, but we don't yet understand the assignability toX:https://github.com/strawberry-graphql/strawberry/blob/main/strawberry/exceptions/handler.py#L90
+ error[lint:invalid-assignment] /tmp/mypy_primer/projects/strawberry/strawberry/exceptions/handler.py:90:9: Object of type `def strawberry_threading_exception_handler(args: tuple[type[BaseException], BaseException | None, TracebackType | None, Thread | None]) -> None` is not assignable to attribute `excepthook` of type `(_ExceptHookArgs, /) -> Any`New true positives
https://github.com/DataDog/dd-trace-py/blob/6bbb5519fe4b3964f9ca73b21cf35df8387618b2/tests/tracer/test_span.py#L714
+ error[lint:invalid-argument-type] /tmp/mypy_primer/projects/dd-trace-py/tests/tracer/test_span.py:714:33: Argument to this function is incorrect: Expected `str`, found `Literal[b"\xf0\x9f\xa4\x94"]`Changed diagnostics
A lot of changed diagnostics because we now show
@Todo(Support fortyping.TypeVarinstances in type expressions)instead ofUnknownfor all kinds of symbols that used a_T = TypeVar("_T")as a type. One prominent example is thelist.__getitem__method:builtins.pyi:which causes this change in diagnostics:
Test Plan
Updated Markdown tests