Skip to content

Add containing union and offending type information when reporting on missing attributes for unions #3402

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

Merged
merged 12 commits into from
May 23, 2017

Conversation

Nurdok
Copy link
Contributor

@Nurdok Nurdok commented May 22, 2017

This is an attempt to address #2561.

For example, for the following code:

from typing import Union

class A: x = 1
class B: pass

v : Union[A, B] = A()
v.x

The previous error was:

Some element of union has no attribute "x"

With this PR, the error is:

Element "B" of "Union[A, B]" has no attribute "x"

Upade: I locally fixed the following None issue in the latest commit.

When dealing with None, mypy reports object. For the example in the above issue:

def foo(s: Optional[str]) -> None:
    print(s.count('a'))

The previous error was:

error: Some element of union has no attribute "count"

With this PR, the error becomes:

Element "object" of "Optional[str]" has no attribute "count"

I guess this happens because mypy searches up the inhertiance tree for NoneType and I'm not sure if there's a way to get the correct type in the code - suggestions welcome.

@ilevkivskyi
Copy link
Member

Thanks for the PR! Just a small comment: I think the situation with None is simple: PEP 484 specifies None as a shorthand for NoneType. I also think it is important to special-case the situation when you have a union of exactly two types and one of them is exactly None in messages.py: you could just print such type as Optional[...].

@Nurdok Nurdok force-pushed the feature/better-union-attr-msg branch from cd1400e to 0ac3f21 Compare May 22, 2017 21:58
@Nurdok
Copy link
Contributor Author

Nurdok commented May 22, 2017

I used the format helper to print nicer type representation, which took care of the Optional thing. I still get "object" when I expect "None" .

@Nurdok Nurdok changed the title WIP: Add containing union and offending type information when reporting on missing attributes for unions Add containing union and offending type information when reporting on missing attributes for unions May 22, 2017
@JukkaL JukkaL self-requested a review May 22, 2017 23:40
@JukkaL
Copy link
Collaborator

JukkaL commented May 22, 2017

Thanks for the PR! Looks pretty good. A few quick comments:

  • I'd prefer Item "foo" of "Union[...]" ... instead of Element "foo" ....
  • Can you add a test where 2 out of 3 items are missing an attribute, and another ones where all 2/2 or 3/3 union items have missing attributes?
  • It would be good to have more tests for things like __getitem__ or __contains__ with a union type, where some or all items don't have the method.

@Nurdok
Copy link
Contributor Author

Nurdok commented May 23, 2017

@JukkaL I addressed your first two points. Regarding magic methods, at least the ones you mentioned result in specialized errors.

Doing 1 in u results in

Unsupported right operand type for in ("Union[C, D]")

And doing u[0] results in

 Value of type "Union[C, D]" is not indexable

It would be desirable for these cases to also result in more specific errors (e.g., Item "C" in "Union[C, D]" is not indexable), but it probably belongs in a separate PR.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

This PR is almost there! I'd like to just see one docstring tweak.

@@ -361,72 +361,79 @@ def format_distinctly(self, type1: Type, type2: Type) -> Tuple[str, str]:
# get some information as arguments, and they build an error message based
# on them.

def has_no_attr(self, typ: Type, member: str, context: Context) -> Type:
def has_no_attr(self, original_type: Type, typ: Type, member: str, context: Context) -> Type:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Document original_type in the docstring.

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@JukkaL JukkaL merged commit 20e41cb into python:master May 23, 2017
@Nurdok Nurdok deleted the feature/better-union-attr-msg branch May 23, 2017 16:48
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.

3 participants