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

Make type inference failures more consistent #7079

Merged
merged 5 commits into from Jun 27, 2019

Conversation

@ilevkivskyi
Copy link
Collaborator

commented Jun 27, 2019

Fixes #6998

This PR makes several related changes:

  • Reserve Cannot determine type error for cases where node deferral failed etc (for example with runtime recursive definitions); don't show it after failed (ambiguous) type inference.
  • Always set reasonable variable type after failed inference (like List[Any] for an empty list).
  • Always give Need type annotation for variable error, not just for instances and tuples (that was ad-hoc IMO)

This may actually give some new errors as compared to status quo, but as one can see from the diff, the result is negative in number of (redundant) errors.

There are couple changes in tests that look unrelated, these are because is_same_type(..., UnboundType()) returns True for everything, including UninhabitedType. I would say we should actually avoid leaking unbound types from semantic analysis and replace the, with Any, but this is a separate issue.

@ilevkivskyi ilevkivskyi requested a review from JukkaL Jun 27, 2019

@JukkaL

JukkaL approved these changes Jun 27, 2019

Copy link
Collaborator

left a comment

Nice, this cleans up type inference logic some. Looks good; left only a few minor comments.

I still don't fully understand what caused the different behavior in new semantic analyzer. Could you explain that in some detail?

@@ -996,7 +996,7 @@ IntNode[int](1, 1)
IntNode[int](1, 'a') # E: Argument 2 to "Node" has incompatible type "str"; expected "int"

SameNode = Node[T, T]
ff = SameNode[T](1, 1) # E: Need type annotation for 'ff'
ff = SameNode[T](1, 1)

This comment has been minimized.

Copy link
@JukkaL

JukkaL Jun 27, 2019

Collaborator

I think that this should generate an error, since T is not in scope here. If this seems unrelated, you can create an issue about this (and add a TODO comment here).

@@ -2716,3 +2716,34 @@ class N(NamedTuple):
)
b
[builtins fixtures/tuple.pyi]

[case testNewAnalyzerLessErrorsNeedAnnotation]

This comment has been minimized.

Copy link
@JukkaL

JukkaL Jun 27, 2019

Collaborator

What about adding a test case for x = [] # type: ignore followed by reveal_type(x)?


class SetNothingToAny(TypeTranslator):
"""Replace all ambiguous <nothing> types with Any (to avoid spurious extra errors)."""
def visit_uninhabited_type(self, t: UninhabitedType) -> Type:

This comment has been minimized.

Copy link
@JukkaL

JukkaL Jun 27, 2019

Collaborator

Style nit: add empty line after class docstring.

def is_valid_inferred_type_component(typ: Type) -> bool:
"""Is this part of a type a valid inferred type?
class NothingSeeker(TypeQuery[bool]):
"""Find any <nothing> types resulting from failed (ambiguous) type inference."""

This comment has been minimized.

Copy link
@JukkaL

JukkaL Jun 27, 2019

Collaborator

Style nit: add empty line after class docstring.

@@ -205,8 +205,7 @@ def h() -> None:
[out]
==
a.py:3: error: Invalid type "b.C"
b.py:6: error: Need type annotation for 'c'
b.py:7: error: Cannot determine type of 'c'
b.py:7: error: C? has no attribute "g"

This comment has been minimized.

Copy link
@JukkaL

JukkaL Jun 27, 2019

Collaborator

Hmm do we now propagate unbound types in type inference? Not sure if that's a good or bad thing.

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Jun 27, 2019

Author Collaborator

Not sure if that's a good or bad thing.

This is rather bad, but I believe the right solution is #4987.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 27, 2019

I still don't fully understand what caused the different behavior in new semantic analyzer. Could you explain that in some detail?

See the discussion in PR #6450 and the change in [case testUnderspecifiedInferenceResult] there.

@ilevkivskyi ilevkivskyi merged commit bbd732f into python:master Jun 27, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@ilevkivskyi ilevkivskyi deleted the ilevkivskyi:new-analyzer-less-errors branch Jun 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.