[ty] Validate PEP 695 type alias scope and redeclaration rules#24341
[ty] Validate PEP 695 type alias scope and redeclaration rules#24341charliermarsh wants to merge 2 commits intomainfrom
Conversation
Typing conformance results improved 🎉The percentage of diagnostics emitted that were expected errors increased from 86.76% to 86.79%. The percentage of expected errors that received a diagnostic increased from 81.53% to 81.72%. The number of fully passing files held steady at 70/132. SummaryHow are test cases classified?Each test case represents one expected error annotation or a group of annotations sharing a tag. Counts are per test case, not per diagnostic — multiple diagnostics on the same line count as one. Required annotations (
Test file breakdown1 file altered
True positives added (2)2 diagnostics
|
Memory usage reportMemory usage unchanged ✅ |
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-type-alias |
1 | 0 | 0 |
| Total | 1 | 0 | 0 |
Raw diff:
AutoSplit (https://github.com/Toufool/AutoSplit)
+ src/utils.py:34:10 error[invalid-type-alias] Type alias `STARTUPINFO` is already defined in this scopeff207d0 to
9f29ce9
Compare
9f29ce9 to
70875ae
Compare
70875ae to
bb6f05b
Compare
| type BranchRedeclared = int | ||
| else: | ||
| # error: [invalid-type-alias] "Type alias `BranchRedeclared` is already defined in this scope" | ||
| type BranchRedeclared = str |
There was a problem hiding this comment.
I was a bit surprised by this, but other type checkers do flag it.
There was a problem hiding this comment.
I don't like flagging this -- does the conformance suite require it?
There was a problem hiding this comment.
If we do flag this, we also need to ensure we don't flag it if one of the branches is statically unreachable.
There was a problem hiding this comment.
I agree with @carljm, but yes -- unfortunately the conformance suite does require it currently: https://github.com/python/typing/blob/1df1565c69730d88ce6877009d268ba1d602af1e/conformance/tests/aliases_type_statement.py#L52. Not sure if that's actually following the spec or if the conformance suite is going beyond the spec there
There was a problem hiding this comment.
I can't find any such requirement in PEP 695 or in the spec. I think we should PR the conformance suite to not require this error.
There was a problem hiding this comment.
Just to make sure I understand -- do you also dislike flagging this?
type Redeclared = int
# error: [invalid-type-alias] "Type alias `Redeclared` is already defined in this scope"
type Redeclared = strOr are you referring to the conditional version?
There was a problem hiding this comment.
it's not consistent with the way we allow this, for example:
x: int = 42
x: str = "42"other type checkers flag that, but we intentionally deviate from other type checkers there -- it's not specified that type checkers should reject that, and the intent of the user to fully shadow the prior variable with a new declaration is clear. So yeah, I dislike flagging Redeclared in your example above, for the same reasons that I've always disliked that other type checkers flag x in my example :-)
There was a problem hiding this comment.
Okay thanks. I kind of thought that snippet was fine to flag, but that the conditional should've been allowed -- at least, that was the intent of my initial comment :) But I defer to you two, your arguments make sense to me.
There was a problem hiding this comment.
I also don't see the rule against defining type aliases in function bodies specified anywhere, and that also seems sorta unnecessary... it's not totally clear to me why users should be prohibited from doing something like this, which seems fine: https://play.ty.dev/f918c85d-8bf3-440c-a5bf-1188cd72d053
Summary
We now detect PEP 695 redeclarations in the same scope, like:
And also declarations within function scopes: