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

Note that assert_type always matches against any in untyped function #13626

Merged
merged 5 commits into from
Sep 8, 2022

Conversation

rsokl
Copy link
Contributor

@rsokl rsokl commented Sep 8, 2022

Description

reveal_type provides a useful note to users that it will always reveal Any within an unchecked function. Similarly, assert_type always checks against Any, but does not notify the user about its behavior.

This PR adds said note, which is displayed when assert_type raises an error in an unchecked function

Test Plan

I added a test to test-data/unit/check-expressions.test that is comparable to the corresponding test case for reveal_type in an unchecked scope.

@@ -3724,6 +3724,10 @@ def visit_assert_type_expr(self, expr: AssertTypeExpr) -> Type:
)
target_type = expr.type
if not is_same_type(source_type, target_type):
if not self.chk.in_checked_function():
self.msg.note(
"'assert_type' always outputs 'Any' in unchecked functions", expr.expr
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is quite right; assert_type() doesn't output anything.

Copy link
Contributor Author

@rsokl rsokl Sep 8, 2022

Choose a reason for hiding this comment

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

Ah, you mean in terms of wording? Would this be better? "assert_type(x, y) always evaluates x as Any in unchecked functions"

Copy link
Contributor Author

@rsokl rsokl Sep 8, 2022

Choose a reason for hiding this comment

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

Oh, I see now.. if doesn't actually output a message in the test.

When I actually run it from my branch I saw

$ mypy func_scope.py 
func_scope.py:5: error: Expression is of type "Any", not "Literal[42]"
func_scope.py:5: note: 'assert_type' always outputs 'Any' in unchecked functions
Found 1 error in 1 file (checked 1 source file)

so it had looked like my fix worked

Copy link
Member

Choose a reason for hiding this comment

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

Yes, your code looks correct in the sense that the note gets output, but I don't think the wording is good. I like @hauntsaninja's suggestion below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotchya!

I am not sure why my test is failing. It looks like it has the same form as the other test cases, but no message is emitted: https://github.com/python/mypy/runs/8241823231?check_suite_focus=true#step:8:295

mypy/checkexpr.py Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@hauntsaninja
Copy link
Collaborator

You need to make sure [builtins fixtures/tuple.pyi] follows the body of the test (this is why your change regresses the previous test). I believe you also need to import assert_type from typing; mypy doesn't pretend it exists in builtins like with reveal_type

@rsokl
Copy link
Contributor Author

rsokl commented Sep 8, 2022

You need to make sure [builtins fixtures/tuple.pyi]

Got it! I updated the note and fixed the test according to your feedback

@github-actions

This comment has been minimized.

@rsokl
Copy link
Contributor Author

rsokl commented Sep 8, 2022

Thanks @sobolevn I addressed your comments in d25a8e1

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@rsokl
Copy link
Contributor Author

rsokl commented Sep 8, 2022

I think that this can be affected by https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-check-untyped-defs

@sobolevn you are right: including --check-untyped-defs` does affect this behavior. The test case that you recommended demonstrates this: https://github.com/python/mypy/runs/8254912870?check_suite_focus=true

I am beyond my depth with working with mypy's internals here, and am afraid that I am wasting mypy dev time by stumbling my way through this. I am happy to close this if it is easier for someone else to take care of this.

@sobolevn
Copy link
Member

sobolevn commented Sep 8, 2022

@rsokl no worries, I am happy to help :)
The first PR is the most important one!

My understanding is that self.chk.in_checked_function() takes --check-untyped-defs into account. So, the test with this option should not raise this warning.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2022

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

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

LGMT, any other comments? :)

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.

Thank you!

@hauntsaninja hauntsaninja merged commit d8652b4 into python:master Sep 8, 2022
@rsokl
Copy link
Contributor Author

rsokl commented Sep 8, 2022

Thank you, all of you, for your help! I really appreciate it

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.

4 participants