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

Fix typeguards crash when assigning guarded value in if statement #10671

Closed
wants to merge 3 commits into from

Conversation

A5rocks
Copy link
Contributor

@A5rocks A5rocks commented Jun 19, 2021

Description

Fixes #10665

This PR implements joining type guard types (which can happen when leaving an if statement). (it continues along the lines of #10496)

Test Plan

Added the crash case the test cases. They all passed locally, except some mypyd ones that complained about restarting, which makes no sense. Hopefully they work in GitHub actions.

@@ -433,7 +433,7 @@ def visit_type_alias_type(self, t: TypeAliasType) -> ProperType:
assert False, "This should be never called, got {}".format(t)

def visit_type_guard_type(self, t: TypeGuardType) -> ProperType:
assert False, "This should be never called, got {}".format(t)
return t.type_guard.accept(self)
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 right, since for join(T, TypeGuard[U]) it will end up with join(T, U), which is approximately Union[T, U] in practice. Computing join(T, bool) may make more sense.

I'm not sure what practical effect this has though. It's at least clear that we'll have to make sure we handle TypeGuard correctly in more places in the codebase.

Copy link
Contributor Author

@A5rocks A5rocks Jun 19, 2021

Choose a reason for hiding this comment

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

The thing is, it's (in the example crash code) saying a is TypeGuard[B] -- when it's actually B (?). (and then it is joining the type with the type of a in the outer scope). However, when I changed that (3f068f5) some of the test cases started erroring... in specific, some of the narrowing ones:

_____________________________________________________________ testTypeGuardNonOverlapping _____________________________________________________________
[gw6] linux -- Python 3.8.2 /home/a5/.virtualenvs/tmp-7d419bcf0dbd427/bin/python
data: /home/a5/oss/mypy/test-data/unit/check-typeguard.test:78:
/home/a5/oss/mypy/mypy/test/testcheck.py:137: in run_case
    self.run_case_once(testcase)
/home/a5/oss/mypy/mypy/test/testcheck.py:236: in run_case_once
    assert_string_arrays_equal(output, a, msg.format(testcase.file, testcase.line))
E   AssertionError: Unexpected type checker output (/home/a5/oss/mypy/test-data/unit/check-typeguard.test, line 78)
---------------------------------------------------------------- Captured stderr call -----------------------------------------------------------------
Expected:
  main:6: note: Revealed type is "builtins.list[builtins.str]" (diff)
Actual:
  main:6: note: Revealed type is "None"         (diff)

Alignment of first line difference:
  E: ...te: Revealed type is "builtins.list[builtins.str]"
  A: ...te: Revealed type is "None"
                              ^
______________________________________________________________ testTypeGuardNonzeroFloat ______________________________________________________________
[gw6] linux -- Python 3.8.2 /home/a5/.virtualenvs/tmp-7d419bcf0dbd427/bin/python
data: /home/a5/oss/mypy/test-data/unit/check-typeguard.test:105:
/home/a5/oss/mypy/mypy/test/testcheck.py:137: in run_case
    self.run_case_once(testcase)
/home/a5/oss/mypy/mypy/test/testcheck.py:236: in run_case_once
    assert_string_arrays_equal(output, a, msg.format(testcase.file, testcase.line))
E   AssertionError: Unexpected type checker output (/home/a5/oss/mypy/test-data/unit/check-typeguard.test, line 105)
---------------------------------------------------------------- Captured stderr call -----------------------------------------------------------------
Expected:
  main:5: note: Revealed type is "builtins.float" (diff)
Actual:
  main:5: note: Revealed type is "builtins.int" (diff)

Alignment of first line difference:
  E: ...ed type is "builtins.float"
  A: ...ed type is "builtins.int"
                             ^
___________________________________________________________ testTypeGuardNarrowToTypedDict ____________________________________________________________
[gw6] linux -- Python 3.8.2 /home/a5/.virtualenvs/tmp-7d419bcf0dbd427/bin/python
data: /home/a5/oss/mypy/test-data/unit/check-typeguard.test:155:
/home/a5/oss/mypy/mypy/test/testcheck.py:137: in run_case
    self.run_case_once(testcase)
/home/a5/oss/mypy/mypy/test/testcheck.py:236: in run_case_once
    assert_string_arrays_equal(output, a, msg.format(testcase.file, testcase.line))
E   AssertionError: Unexpected type checker output (/home/a5/oss/mypy/test-data/unit/check-typeguard.test, line 155)
---------------------------------------------------------------- Captured stderr call -----------------------------------------------------------------
Expected:
  main:10: note: Revealed type is "TypedDict('__main__.User', {'name': builtins.str, 'id': builtins.int})" (diff)
Actual:
  main:10: note: Revealed type is "None"        (diff)

Alignment of first line difference:
  E: ...ote: Revealed type is "TypedDict('__main__.User', {'name': builtins.s...
  A: ...ote: Revealed type is "None"...
                               ^

Because of that, I kind of assumed that TypeGuard[T] is just T -- I guess that isn't the case though? If so, then this might be more annoying to fix...

@A5rocks
Copy link
Contributor Author

A5rocks commented Jun 19, 2021

Alright, so I'm somewhat confused on how to approach this. TypeGuardType seems to be used for both type guard functions (?) and type guarded variables. This makes sense since type guarded variables are a little special, but at the same time is massively confusing. See for example:

mypy/mypy/checkexpr.py

Lines 4178 to 4181 in 790ab35

# Ignore the error about using get_proper_type().
if isinstance(restriction, TypeGuardType): # type: ignore[misc]
# A type guard forces the new type even if it doesn't overlap the old.
return restriction.type_guard

This was why my initial approach of saying type guarded variables were the same type as their actual type didn't work, because it would not just take the new type, it would instead apply some narrowing.

However, if a TypeGuard[T] is a TypeGuardType(T), then something has to change. I'm unsure on how to resolve this -- maybe I could add a boolean to TypeGuardType saying whether it is a guarded value? Maybe make an entirely new ProperType (which seems like a pain to do)? I'd love a bit of guidance!

@JukkaL
Copy link
Collaborator

JukkaL commented Jun 21, 2021

I'm pretty sure that we shouldn't leak type guard types into various type operations. If we try to join a type guard type, the bug is probably elsewhere.

JukkaL added a commit that referenced this pull request Jun 21, 2021
This is a quick fix to unblock the 0.910 release.

The type guard type can be unrelated to the original type, so we
shouldn't join it. I'm not sure whether this is right from first
principles, but it seems to address the issue.

Fixes #10671.
@JukkaL
Copy link
Collaborator

JukkaL commented Jun 21, 2021

FWIW, #10683 fixes the crash in a simple but kind of hacky way. A better understanding of what's going on would be nice. Maybe the binder should handle type guard types in a different way.

JukkaL added a commit that referenced this pull request Jun 21, 2021
This is a quick fix to unblock the 0.910 release.

The type guard type can be unrelated to the original type, so we
shouldn't join it. I'm not sure whether this is right from first
principles, but it seems to address the issue.

Fixes #10671.
JukkaL added a commit that referenced this pull request Jun 22, 2021
This is a quick fix to unblock the 0.910 release.

The type guard type can be unrelated to the original type, so we
shouldn't join it. I'm not sure whether this is right from first
principles, but it seems to address the issue.

Fixes #10671.
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.

Crash on assigning to guarded variable in failed TypeGuard branch
3 participants