Skip to content

[red-knot] Don't infer Todo for quite so many tuple type expressions#17116

Merged
AlexWaygood merged 1 commit intomainfrom
alex/redknot-tuples
Apr 1, 2025
Merged

[red-knot] Don't infer Todo for quite so many tuple type expressions#17116
AlexWaygood merged 1 commit intomainfrom
alex/redknot-tuples

Conversation

@AlexWaygood
Copy link
Member

Summary

I noticed we were inferring Todo as the declared type for annotations such as x: tuple[list[int], list[int]]. This PR reworks our annotation parsing so that we instead infer tuple[Todo, Todo] for this annotation, which is quite a bit more precise.

Test Plan

Existing mdtest updated.

@AlexWaygood AlexWaygood added the ty Multi-file analysis & type inference label Apr 1, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Apr 1, 2025

mypy_primer results

Changes were detected when running on open source projects
black (https://github.com/psf/black)
+ error[lint:invalid-return-type] /tmp/mypy_primer/projects/black/src/blib2to3/pgen2/pgen.py:187:16: Object of type `tuple[dict, None | Unknown]` is not assignable to return type `tuple[@Todo(generics), str]`
- Found 211 diagnostics
+ Found 212 diagnostics

@AlexWaygood
Copy link
Member Author

link to the primer hit: https://github.com/psf/black/blob/2c135edf3732a8efcc89450446fcaa7589e2a1c8/src/blib2to3/pgen2/pgen.py#L164-L187

I think it's probably a false positive, but a single false positive is probably worth it here IMO. This gives us better understanding for a lot of tuple annotations that we were giving up on parsing before.

Copy link
Member

@dcreager dcreager left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's probably a false positive, but a single false positive is probably worth it here IMO. This gives us better understanding for a lot of tuple annotations that we were giving up on parsing before.

There's an assert at the end of that function that should narrow away the None from the second element of the return tuple. If it's quick to figure out why that's happening, I think it would be worth tackling here. But if it's unrelated to the tuple change (i.e. do we also not narrow None | Unknown to Unknown correctly on its own?) then I agree that the one false positive is worth it here.

@AlexWaygood
Copy link
Member Author

Ah, we don't do type narrowing for assert statements yet! So that explains it.

@AlexWaygood AlexWaygood merged commit 49c2599 into main Apr 1, 2025
23 checks passed
@AlexWaygood AlexWaygood deleted the alex/redknot-tuples branch April 1, 2025 14:44
dcreager added a commit that referenced this pull request Apr 1, 2025
* main:
  [red-knot] Add property tests for callable types (#17006)
  [red-knot] Disjointness for callable types (#17094)
  [red-knot] Flatten `Type::Callable` into four `Type` variants (#17126)
  mdtest.py: do a full mdtest run immediately when the script is executed (#17128)
  [red-knot] Fix callable subtyping for standard parameters (#17125)
  [red-knot] Fix more `redundant-cast` false positives (#17119)
  Sync vendored typeshed stubs (#17106)
  [red-knot] support Any as a class in typeshed (#17107)
  Visit `Identifier` node as part of the `SourceOrderVisitor` (#17110)
  [red-knot] Don't infer Todo for quite so many tuple type expressions (#17116)
  CI: Run pre-commit on depot machine (#17120)
  Error instead of `panic!` when running Ruff from a deleted directory (#16903) (#17054)
  Control flow graph: setup (#17064)
  [red-knot] Playground improvements (#17109)
  [red-knot] IDE crate (#17045)
  Update dependency vite to v6.2.4 (#17104)
  [red-knot] Add redundant-cast error (#17100)
  [red-knot] Narrowing on `in tuple[...]` and `in str` (#17059)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants