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

Use TypeVar defaults instead of Any when fixing TypeAlias types (PEP 696) #16825

Merged
merged 2 commits into from
Jan 28, 2024

Conversation

cdce8p
Copy link
Collaborator

@cdce8p cdce8p commented Jan 26, 2024

This PR applies the TypeVar defaults to TypeAlias types instead of using Any exclusively, similar to #16812.
Again TypeVarTuple defaults aren't handled correctly yet.

Ref: #14851

This comment has been minimized.

TA1 = Dict[T2, T3]

def func_a1(
a: TA1, # E: Missing type parameters for generic type "TA1"
Copy link
Member

Choose a reason for hiding this comment

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

This errors seems like a false positive, since both of the type parameters have a default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! I added the --disallow-any-generics flag specifically to detect it, but somehow missed to check it. Pushed a new commit.

Copy link
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Looks good, just some nits

b: TA4[float], # E: Bad number of arguments for type alias, expected between 2 and 3, given: 1
c: TA4[float, float],
d: TA4[float, float, float],
e: TA4[float, float, float, float], # E: Bad number of arguments for type alias, expected between 2 and 3, given: 4
Copy link
Member

Choose a reason for hiding this comment

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

nit: I don't think there should be a colon after "given". However, this looks to be consistent with a bunch of other errors, so we can leave it for now.

A similar runtime error for comparison:

python -c '''
def f(a: int, b: str = ""): ...
f(1, "x", 3)
'''
Traceback (most recent call last):
  File "<string>", line 3, in <module>
TypeError: f() takes from 1 to 2 positional arguments but 3 were given

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, however for this I tried to modify the existing message as little as possible, thus keeping the colon. If you like, I would suggest that I open a separate PR later to update all cases for "Bad number of arguments for type alias". That would be better since it requires changes to other test files as well.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, sounds good!

reveal_type(e) # N: Revealed type is "Tuple[Any, Any, builtins.int]"
[builtins fixtures/dict.pyi]

[case testTypeVarDefaultsTypeAlias2]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[case testTypeVarDefaultsTypeAlias2]
[case testParamSpecDefaultsTypeAlias]

More informative test case name

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To be honest, I try to keep the same prefix for all cases to be able to do pytest -k testTypeVarDefaults. Maybe I'm a bit lazy in that regard 😅 Running the whole test file would also work but it's more to type.

pytest mypy/test/testcheck.py::TypeCheckSuite::check-typevar-defaults.test

--
IMO it also makes reading the test cases a bit easier since you can quickly see that all deal with ...TypeAlias. Similarly to the last PR were all used ...Class.

--
In the end it's probably just preference, so if you like me to I'll change it.

reveal_type(d) # N: Revealed type is "__main__.ClassB2[Any, [builtins.int, builtins.str]]"
[builtins fixtures/tuple.pyi]

[case testTypeVarDefaultsTypeAlias3]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[case testTypeVarDefaultsTypeAlias3]
[case testTypeVarTupleDefaultsTypeAlias]

@JelleZijlstra JelleZijlstra merged commit 09490c8 into python:master Jan 28, 2024
18 checks passed
@cdce8p cdce8p deleted the TypeVar-06-fix-typealias branch January 28, 2024 07:46
hauntsaninja pushed a commit that referenced this pull request Jan 30, 2024
Small update to adjust the error messages following the suggestion in
#16825 (comment).
@cdce8p cdce8p added the topic-pep-696 TypeVar defaults label Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-pep-696 TypeVar defaults
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants