-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Handle member access on Type[Any], fix #3098 #3246
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
Conversation
[case testTypeUsingTypeCTypeAnyMember] | ||
from typing import Type, Any | ||
def foo(arg: Type[Any]): | ||
x = arg.member_name |
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.
Can you also try an assignment to an arbitrary member?
We could perhaps at least give a precise type for things like arg.mro
which are defined in type
. Maybe only fall back to Any
if type
doesn't have the attribute? If this is hard to do it's okay to leave it out from this PR.
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 can add a test with an assignment into a member of Type[Any]
(from what I see, works with no changes). Is that what you're asking for?
I tried doing the fallback to type
and it seems to work with small examples outside the test suite, but I'm having trouble writing the tests for that. Apparently reveal_type(type.mro)
does different thing outside and inside the test suite (inside the test suite I get Any
instead of def () -> builtins.list[builtins.type]
) Should I do something special to get the right behaviour inside the test suite?
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.
That is because outside the test suite it uses type.mro from typeshed, which is defined as def mro(self) -> List[type]: ...
. Presumably the test fixtures don't have that method defined.
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 added the assignment in a new commit to the PR, if you want to accept it as-is.
My attempt to implement the fallback is in dmoisset@b6d6548 , outside this PR ... it is failing the tests as I mentioned (but appears to be working fine in some external test cases). I understand your explanation @JelleZijlstra , but I'm not certain how to fix the issue (i.e, what to do to get typeshed methods available in the test suite, and whether that is a good idea or if I should write the test in a different way).
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.
@dmoisset You could update one of the builtins fixtures by adding the correct type signature from typeshed, and use it with e.g. [builtins fixtures/list.pyi]
at the end of test case.
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.
Thank you @ilevkivskyi , using that I just updated the PR and now it handles fallback to builtins.type attribute if the attribute is present there
ignore_messages.disable_errors() | ||
return analyze_member_access(name, fallback, node, is_lvalue, is_super, | ||
is_operator, builtin_type, not_ready_callback, | ||
ignore_messages, original_type=original_type, chk=chk) |
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.
What is the reason to not also pass override_info=override_info
?
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 followed the logic used in the rest of the function for fallbacks that generally does not pass it; I thought it was only relevant for Instance
types, but I might be wrong
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.
Looks good -- thanks for the fix!
This should fix #3098