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

stubtest: if a default is present in the stub, check that it is correct #14085

Merged
merged 8 commits into from
Nov 25, 2022

Conversation

JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Nov 13, 2022

Helps with python/typeshed#8988.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks, had one quick comment!

I guess given we want to restrict to simple values in typeshed, the biggest hole here is enums. You can get at the symbol table using get_stub, but you'll probably have to look up the runtime to get the actual enum value.

mypy/stubtest.py Outdated
@@ -573,6 +753,23 @@ def _verify_arg_default_value(
f"has a default value of type {runtime_type}, "
f"which is incompatible with stub argument type {stub_type}"
)
if runtime_arg.default is not ... and stub_arg.initializer is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we have runtime_arg.default is not ... here? (clause looks untested as well)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I don't think I need that check. Will remove.

@JelleZijlstra
Copy link
Member Author

Not sure how to fix the mypyc failure.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Maybe try class _NodeEvaluator(object, ExpressionVisitor[object]):?

If that doesn't work, maybe just preserve the generic: class _NodeEvaluator(Generic[T], ExpressionVisitor[T]):

mypy/stubtest.py Outdated
@@ -540,6 +543,186 @@ def names_approx_match(a: str, b: str) -> bool:
)


class _NodeEvaluator(ExpressionVisitor[object]):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
class _NodeEvaluator(ExpressionVisitor[object]):
class _NodeEvaluator(object, ExpressionVisitor[object]):

Copy link
Member Author

Choose a reason for hiding this comment

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

Tried that, it doesn't even work without mypyc because it creates an inconsistent MRO. I'll try the generic way you suggested instead.

@JelleZijlstra
Copy link
Member Author

Now it segfaults instead. Progress!

@github-actions

This comment has been minimized.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Oh interesting. I guess we'll see what happens when we need to hook into more stubtest things to get enums to work out. There are enough "unknown"s here that I wouldn't feel bad about using an if statement instead of a visitor.

Feel free to merge when you get things green!

@JelleZijlstra
Copy link
Member Author

Oh interesting. I guess we'll see what happens when we need to hook into more stubtest things to get enums to work out. There are enough "unknown"s here that I wouldn't feel bad about using an if statement instead of a visitor.

Yes, a visitor feels like the clean solution here but with so many unknowns, it's pretty ugly.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JelleZijlstra JelleZijlstra merged commit a9c62c5 into python:master Nov 25, 2022
@JelleZijlstra JelleZijlstra deleted the stubdefault branch November 25, 2022 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants