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

[red-knot] Introduce type parameters for InstanceType #15183

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

pcmoritz
Copy link

@pcmoritz pcmoritz commented Dec 30, 2024

Summary

This PR introduces type parameters to InstanceType, so an instance of type like dict[str, str] can be represented.

I'm curious for feedback if this is the right way to do it or if there is a better way!

Test Plan

Copy link
Contributor

github-actions bot commented Dec 30, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@MichaReiser MichaReiser added the red-knot Multi-file analysis & type inference label Dec 30, 2024
@AlexWaygood
Copy link
Member

Hey, thanks for this PR! Sorry for the delay in getting back to you on this -- several people on the team were on holiday between Christmas and new year's day :-)

Unfortunately, how best to represent generic type parameters and generic type arguments is quite a big topic that requires a certain amount of care. @carljm has written up a plan for how to do this which has been shared and discussed internally, and it will require a fair amount of refactoring to get all the details right. This is because we need to make sure that e.g. an error is emitted if a user uses dict[str, int, bytes] in an annotation (dict requires exactly two type arguments).

Additionally, it's quite hard for us to test our implementation of generics until we properly implement attribute access on Type::Instance types. This is another huge topic, and one that @sharkdp is planning on tackling -- but it probably needs to be done before generics, unfortunately!

All in all, I think it's probably unlikely that we'll merge this, sadly. It's not a great task for a contributor at the end of the day -- but we have lots of red-knot issues with the "help wanted" label open at the moment; I'd encourage you to take a look at those and see if any take your fancy!

@carljm
Copy link
Contributor

carljm commented Jan 5, 2025

Thanks for your work on this! As @AlexWaygood suggests, this isn't quite the direction we'll want to take. Specifically, we'll want to represent a "specialized" generic class (that is, a generic class with its type parameters filled in with type arguments) at a layer below this: roughly, a new layer of struct wrapping Class and wrapped by InstanceType. This is because a specialized generic class can also occur in other places where we currently have a Class: not only InstanceType, but also SubclassOfType, ClassBase::Class, and maybe others I'm forgetting at the moment.

Although this isn't the topic I'd have suggested for a first contribution to red-knot (because it will be an invasive change), it's promising that you carried this initial approach this far successfully! If you're highly motivated to work on this contribution, and willing to likely go through several review iterations to get there, I'd be open to sharing the internal design doc with you (which fleshes out the above sketch with specific struct names and more details) and working with you on it as reviewer. Let me know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants