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.

Sign up for GitHub

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

Jump to bottom

Add foundation for TypeVar defaults (PEP 696) #14872

Merged
merged 3 commits into from
May 29, 2023
Merged

Add foundation for TypeVar defaults (PEP 696) #14872

merged 3 commits into from
May 29, 2023

Conversation

cdce8p
Copy link
Collaborator

@cdce8p cdce8p commented Mar 11, 2023 •
edited
Loading

Start implementing PEP 696 TypeVar defaults. This PR

  • Adds a default parameter to TypeVarLikeExpr and TypeVarLikeType.
  • Updates most visitors to account for the new default parameter.
  • Update existing calls to add value for default => AnyType(TypeOfAny.from_omitted_generics).

A followup PR will update the semantic analyzer and add basic tests for TypeVar, ParamSpec, and TypeVarTuple calls with a default argument. -> #14873

Ref #14851

@cdce8p cdce8p mentioned this pull request Mar 11, 2023
Merged
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@hauntsaninja hauntsaninja requested a review from JukkaL May 2, 2023 18:22
JelleZijlstra
JelleZijlstra reviewed May 5, 2023
View reviewed changes
mypy/semanal.py Outdated
# Similar to regular (user defined) type variables.
self.process_placeholder(
None,
"Self upper bound",
"Self upper bound or default",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe better to have a separate message for the two cases?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this particular instance I believe it isn't even necessary to check it at all. For Self the default is always set to AnyType(TypeOfAny.from_omitted_generics) and thus can't include a placeholder. I've split the calls in process_typevar_declaration though.

Also noticed that I was missing these checks in process_paramspec_declaration and process_typevartuple_declaration which I added now.

@github-actions

This comment has been minimized.

Sign in to view
@cdce8p cdce8p mentioned this pull request May 10, 2023
Closed
@github-actions

This comment has been minimized.

Sign in to view
JelleZijlstra
JelleZijlstra approved these changes May 29, 2023
View reviewed changes
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, one suggestion

mypy/nodes.py Show resolved Hide resolved
@github-actions
Copy link
Contributor

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

@JelleZijlstra JelleZijlstra merged commit a568f3a into python:master May 29, 2023
@Dreamsorcerer Dreamsorcerer mentioned this pull request May 29, 2023
Open
1 task
@cdce8p
Copy link
Collaborator Author

cdce8p commented May 29, 2023

Thanks @JelleZijlstra! I'll take a closer look at my followup PR tomorrow.
Would you mind creating a new label, something like topic-pep-696? That would help keep track of PRs and upcoming issues.

@JelleZijlstra
Copy link
Member

Done: topic-pep-696 TypeVar defaults . I also just fixed the merge on #14873 (possibly incorrectly, there's some self-check failures) and added some comments.

@JelleZijlstra JelleZijlstra added the topic-pep-696 TypeVar defaults label May 29, 2023
@cdce8p cdce8p deleted the TypeVar-01-foundation branch May 29, 2023 23:55
@cdce8p cdce8p mentioned this pull request May 30, 2023
Merged
5 tasks
@VincentVanlaer VincentVanlaer mentioned this pull request Jun 15, 2023
Merged
@cdce8p cdce8p mentioned this pull request Jun 21, 2023
Open
@fjarri fjarri mentioned this pull request Jun 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

@JukkaL JukkaL Awaiting requested review from JukkaL

Assignees
No one assigned
Labels
topic-pep-696 TypeVar defaults
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cdce8p @JelleZijlstra