GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-102615: Fix type vars substitution of `collections.abc.Callable` and custom generics with `ParamSpec` by sobolevn · Pull Request #102681 · python/cpython

Conversation

@sobolevn

@sobolevn sobolevn commented Mar 14, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

I would prefer to create two PRs to fix #102615

  1. This one that fixes a bug with Callable and custom generics with ParamSpec
  2. gh-102615: Use list instead of tuple in repr of paramspec #102637 to change the repr
  • Issue: Representation of ParamSpecs at runtime compared to Callable #102615

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@sobolevn
pythongh-102615: Fix type vars substitution of `collections.abc.Calla…
2731844
…ble` and custom generics with `ParamSpec`
@sobolevn
sobolevn requested review from AlexWaygood, Fidget-Spinner, JelleZijlstra, gvanrossum and rhettinger as code owners March 14, 2023 07:14
@bedevere-bot bedevere-bot mentioned this pull request Mar 14, 2023
Representation of ParamSpecs at runtime compared to Callable #102615
Closed
@bedevere-bot bedevere-bot added the awaiting review label Mar 14, 2023
@sobolevn sobolevn mentioned this pull request Mar 14, 2023
gh-102615: Use list instead of tuple in repr of paramspec #102637
Merged
Fidget-Spinner
Fidget-Spinner reviewed Mar 14, 2023
View reviewed changes
Comment thread Lib/_collections_abc.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn
Address review
a461b10
@Fidget-Spinner

Fidget-Spinner commented Mar 14, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

I encountered another bug, the __repr__ needs to be updated as well:

>>> ca.Callable[[int, P], int][P][int]
collections.abc.Callable[[int, (<class 'int'>,)], int]

EDIT: THIS MIGHT NOT BE A BUG, AS ITS AN INVALID USE. PLEASE IGNORE THIS @sobolevn

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn

sobolevn commented Mar 14, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

@Fidget-Spinner I would like to move all repr related problems to another PR if that possible :)

I've only touched params substitution here.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn

sobolevn commented Mar 14, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Wait a moment, this is not about repr, this is an __args__ containing a tuple 😒

>>> ca.Callable[[int, P], int][P].__args__
(<class 'int'>, ~P, <class 'int'>)
>>> ca.Callable[[int, P], int][P][int].__args__
(<class 'int'>, (<class 'int'>,), <class 'int'>)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rhettinger
rhettinger removed their request for review March 14, 2023 13:42
@sobolevn

sobolevn commented Mar 14, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Funny thing, I just removed this code from _collections_abc.py as an experiment:

if (len(self.__parameters__) == 1
                and _is_param_expr(self.__parameters__[0])
                and item and not _is_param_expr(item[0])):
            item = (item,)

All tests still pass :)

🤯

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood
AlexWaygood reviewed Mar 14, 2023
View reviewed changes

@AlexWaygood AlexWaygood left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think the runtime is correct in raising TypeError for these situations, actually: see #102615 (comment)

Maybe instead, we should focus on improving the error message, so that users get more specific feedback on why the substitution is being disallowed? Though if it adds too much complexity to the code, I probably wouldn't bother with it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@sobolevn

sobolevn commented Mar 14, 2023

Copy link
Copy Markdown
Member Author

I am going to close this for now, I will add new tests in other PRs, because this one has a big unrelated history.

AlexWaygood reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn sobolevn closed this Mar 14, 2023
@sobolevn sobolevn mentioned this pull request Mar 15, 2023
gh-102721: Improve coverage of _collections_abc._CallableGenericAlias #102722
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fidget-Spinner Fidget-Spinner Fidget-Spinner left review comments
@AlexWaygood AlexWaygood AlexWaygood left review comments
@gvanrossum gvanrossum Awaiting requested review from gvanrossum
@JelleZijlstra JelleZijlstra Awaiting requested review from JelleZijlstra JelleZijlstra is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Representation of ParamSpecs at runtime compared to Callable

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@sobolevn @Fidget-Spinner @AlexWaygood @bedevere-bot
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.