-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add more details for "Invalid type" errors #7166
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! This improves one of the most common (and most confusing ) error messages.
This is the first part of my review: I left various suggestions about how to improve the error messages. I'll do another pass once the error messages are final.
test-data/unit/check-basic.test
Outdated
import mock | ||
from typing import Union | ||
|
||
class A: ... | ||
class B: ... | ||
|
||
x: Union[mock, A] # E: Invalid type "mock" | ||
x: Union[mock, A] # E: Invalid type "mock": Module cannot be used as type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bikeshedding: this error message could be shortened down to Module "mock" is not valid as a type
without loss of clarity, I think.
test-data/unit/check-classes.test
Outdated
import six | ||
class M(type): pass | ||
class A(object): pass | ||
def f() -> type: return M | ||
class C1(six.with_metaclass(M), object): pass # E: Invalid base class | ||
class C2(C1, six.with_metaclass(M)): pass # E: Invalid base class | ||
class C1(six.with_metaclass(M), object): pass # E: Invalid base class "six.with_metaclass": Unsupported dynamic base class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to above, this could be shortened to Unsupported dynamic base class "six.with_metaclass"
.
test-data/unit/check-columns.test
Outdated
def f(x: bad): # E:10: Invalid type "__main__.bad" | ||
y: bad # E:8: Invalid type "__main__.bad" | ||
def f(x: bad): # E:10: Invalid type "__main__.bad": Cannot use variable as type | ||
y: bad # E:8: Invalid type "__main__.bad": Cannot use variable as type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like above, this could be shortened to Variable "__main__.bad" is not valid as a type
.
test-data/unit/check-functions.test
Outdated
@@ -1737,12 +1738,12 @@ J = Callable[[VarArg(), KwArg()], int] # ok | |||
K = Callable[[VarArg(), int], int] # E: Required positional args may not appear after default, named or var args | |||
L = Callable[[Arg(name='x', type=int)], int] # ok | |||
# I have commented out the following test because I don't know how to expect the "defined here" note part of the error. | |||
# M = Callable[[Arg(gnome='x', type=int)], int] E: Invalid type alias E: Unexpected keyword argument "gnome" for "Arg" | |||
# M = Callable[[Arg(gnome='x', type=int)], int] E: Invalid type alias: cannot interpret right hand side as a type E: Unexpected keyword argument "gnome" for "Arg" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bikeshedding: Invalid type alias: expression is not a valid type
.
@@ -2195,9 +2196,9 @@ from b import x | |||
1 + 1 | |||
[out] | |||
[out2] | |||
tmp/b.py:2: error: Invalid type "c.C" | |||
tmp/b.py:2: error: Invalid type "c.C": Cannot use function as type, use Callable[...] or a protocol instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, the first part could be shortened to Function "c.C" is not valid as a type
. I'd move the final part to a separate note and rephrase it, since it's not clear of Callable[...]
or a protocol is actually what the user wants. Maybe something like this:
b.py:2: error: Function "c.C" is not a valid type
b.py:2: note: Perhaps you need Callable[...] or a callback protocol?
test-data/unit/check-literal.test
Outdated
from typing_extensions import Literal | ||
a: (1, 2, 3) # E: Syntax error in type annotation \ | ||
# N: Suggestion: Use Tuple[T1, ..., Tn] instead of (T1, ..., Tn) | ||
b: Literal[[1, 2, 3]] # E: Parameter 1 of Literal[...] is invalid | ||
c: [1, 2, 3] # E: Invalid type | ||
c: [1, 2, 3] # E: Invalid type: Did you want to use "List[...]"? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we give a suggestion, we typically give it as a separate line. For example:
x.py:2: error: Expression "[...]" is not a valid type
x.py:2 note: Did you mean "List[t]"?
We could do the same thing for x: (int, str)
(Tuple[t1, ..., tN]
), x: {int}
(Set[t]
) and x: {int: str}
(Dict[kt, vt]
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already do this for tuples, dict and set are more tricky (and I think more rare) so I would leave this for a separate PR.
test-data/unit/check-newtype.test
Outdated
main:4: error: Invalid type "__main__.T" | ||
main:4: error: Argument 2 to NewType(...) must be subclassable (got T?) | ||
main:4: error: Invalid type "__main__.T": Can only use bound type variables as types | ||
main:5: error: Invalid type "__main__.T": Can only use bound type variables as types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bikeshedding: Type variable "__main__.T" is unbound
. Maybe also add a note about what this means, since unbound/bound can be confusing for new users. Idea:
main:4: error: Type variable "__main__.T" is unbound
main:4:note: (Hint: Use "Generic[T]" or "Protocol[T]" base class to bind "T" inside a class)
main:4:note: (Hint: Use "T" in function signature to bind "T" inside a function)
test-data/unit/check-statements.test
Outdated
from typing import Union | ||
class A: | ||
def __enter__(self) -> int: pass | ||
def __exit__(self, x, y, z): pass | ||
|
||
with A(): # type: int # E: Invalid type comment | ||
with A(): # type: int # E: Invalid type comment: `with` statement has no targets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: use double quotes (... "with" statement ...
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! Looks good!
Fixes #4030
This adds some more details to various
Invalid type
errors, and similar such asInvalid type alias
, andInvalid base class
. Unfortunately,MessageBuilder
is not available intypeanal.py
, so I put some error message formatting logic there.Please add ideas about how to improve these errors.