Skip to content

Improved error message for incompatible default argument #3773

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

Closed
wants to merge 1 commit into from

Conversation

elazarg
Copy link
Contributor

@elazarg elazarg commented Jul 28, 2017

When a default argument is incompatible with the parameter, the current error message is

Incompatible types in assignment (expression has type "B", variable has type "A")

Which is slightly confusing, and does not include the name of the parameter. I suggest changing it to something like

Incompatible default argument for parameter "x" (initializer has type "B", parameter has type "A")

Although I'm sure there are better ways to phrase this.

@gvanrossum
Copy link
Member

gvanrossum commented Jul 28, 2017 via email

@elazarg
Copy link
Contributor Author

elazarg commented Jul 28, 2017

Changed to

Incompatible default initializer for argument "x" (initializer has type "B", argument has type "A")

@elazarg
Copy link
Contributor Author

elazarg commented Jul 28, 2017

I'm not sure why does travis fail for 3.3:

pkg_resources.VersionConflict: (pytest 2.9.2 (/home/travis/virtualenv/python3.3.6/lib/python3.3/site-packages), Requirement.parse('pytest>=3.0.0'))

@emmatyping
Copy link
Member

#3777 fixes the pytest error

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I like this, but I'd like to tweak the error message some more. I'd also like to see an explicit test for the Python 2 syntax where an argument can be a tuple, e.g.

def foo((x, y) = (1, 1)): pass

and some examples of errors there (an incorrect type, and too many or too few values).

mypy/checker.py Outdated
@@ -742,7 +742,10 @@ def is_implicit_any(t: Type) -> bool:
for arg in item.arguments:
init = arg.initialization_statement
if init:
self.accept(init)
fmt = 'Incompatible default initializer for argument "{}"'
Copy link
Member

Choose a reason for hiding this comment

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

To shorten the message a bit, I'd suggest leaving out "initializer" here and replacing that word with "default" three lines below.

@gvanrossum
Copy link
Member

Also, if you rebase, the Travis error should be gone.

@gvanrossum
Copy link
Member

Ping?

@elazarg
Copy link
Contributor Author

elazarg commented Jul 31, 2017

Sorry. I've been working on this, but (as is usual for me) got carried away. What I have now is a check using an ad-hoc parameter passing instead, and a complete removal of Argument.initialization_statement and Argument.type_annotation since it's not used.

I think these changes are nice, since they clean up some boilerplate, avoid using new top_frame_context in the argument check, and revealed an escaped UnboundType; they also not immediately what you've asked for, and introduce bigger code churn which is not immediately relevant to this PR, so I did not push them.

I'm still trying to figure out how to take only what is needed, and how to get the parameter names correctly in the general case of tuple arguments (instead of having error messages about __tuple_arg_1 having type Tuple[int, Tuple[str, int]] but default has type Tuple[str, Tuple[str, int]]. It shouldn't be too hard, but I'd like to find "the right way" to do it.

I can push the bigger change if you want to see it (it's bigger than necessary, but not huge), or continue to work on the smaller one, which was my intention.

@elazarg elazarg force-pushed the error-default-arg branch from a374846 to 0413a0e Compare July 31, 2017 08:43
@gvanrossum
Copy link
Member

gvanrossum commented Jul 31, 2017 via email

@elazarg elazarg force-pushed the error-default-arg branch from 0413a0e to 2d03d48 Compare July 31, 2017 14:44
@elazarg elazarg force-pushed the error-default-arg branch from 2d03d48 to 6c89c13 Compare July 31, 2017 14:45
@elazarg
Copy link
Contributor Author

elazarg commented Jul 31, 2017

I have actually pushed the wording changes, unless it's not what you meant:

Incompatible default for argument "x" (argument has type "A", default has type "B")

And the error message for tuples is not as clear but I think is passable:

Incompatible default for tuple argument no' 1 (argument has type "Tuple[B, B]", default has type "Tuple[A, B]")

I also kept the use of call expression instead of assignment. I think it's better, but I can revert (this is not the big change).

@gvanrossum
Copy link
Member

Sigh. You destroyed the github review history by squashing all your commits. I'm annoyed and am going to close this PR. Please start over with just what I requested without more refactoring.

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