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

Rename TypeAndQualifiers to DeclaredType #15969

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dhruvmanila
Copy link
Member

Summary

This PR renames TypeAndQualifiers to DeclaredType.

The main motivation for this rename is that in #15848 I need to pass the ReExport information around and TypeAndQualifiers as a name is limited to only include the TypeQualifiers. The re-export information is relevant only for a declared type.

Another potential name that I thought of was TypeWithMetadata which doesn't tie this with using it only in the declaration sites.

@dhruvmanila dhruvmanila added the red-knot Multi-file analysis & type inference label Feb 5, 2025
@AlexWaygood
Copy link
Member

Hmm -- I'm not sure that type qualifiers will always only be relevant for declared types. We hope to at some point support "implicit class variables" -- there's some TODOs in our tests, such as here:

We also consider a class variable to be a pure class variable if it is only mentioned in a class
method.
```py
class C:
@classmethod
def class_method(cls):
cls.pure_class_variable = "value set in class method"
# for a more realistic example, let's actually call the method
C.class_method()
# TODO: We currently plan to support this and show no error here.
# mypy shows an error here, pyright does not.
# error: [unresolved-attribute]
reveal_type(C.pure_class_variable) # revealed: Unknown

@sharkdp might have some more context on whether the plan is to support these in a similar way to our current support for explicit ClassVars or not

@sharkdp
Copy link
Contributor

sharkdp commented Feb 5, 2025

@sharkdp might have some more context on whether the plan is to support these in a similar way to our current support for explicit ClassVars or not

Hm, yes. I'm also not sure if we'll use (a potentially renamed) TypeAndQualifiers for this.

I don't have strong feelings about the name of this struct, but DeclaredType seems a bit too narrow for me. The declared type is pretty much what is stored in the inner: Type<…> field. Maybe AnnotatedType? Following the distinction between type expressions and annotations expressions in the syntax?

@dhruvmanila
Copy link
Member Author

I don't have strong feelings about the name of this struct, but DeclaredType seems a bit too narrow for me. The declared type is pretty much what is stored in the inner: Type<…> field. Maybe AnnotatedType? Following the distinction between type expressions and annotations expressions in the syntax?

Yeah, I think that makes sense. Although, I've realized that I might need to pass the ReExport information via the bindings as well i.e., augment the binding Type with additional information. I'm going to put this on hold until that is finalized.

@dhruvmanila dhruvmanila marked this pull request as draft February 6, 2025 04:46
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.

3 participants