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 crash when annotating callable definition location #3375

Merged
merged 3 commits into from May 22, 2017

Conversation

Projects
None yet
4 participants
@miedzinski
Contributor

miedzinski commented May 16, 2017

Fixes #3362.

@miedzinski

This comment has been minimized.

Show comment
Hide comment
@miedzinski

miedzinski May 16, 2017

Contributor

I have no idea how to add test case for this.

[case testUnexpectedKwargsDefinedHere]
import m
m.A(x=1)  # E: Unexpected keyword argument "x" for "A"
[file m.py]
class A:
    def __init__(self) -> None:  # N: "A" defined here
        pass

doesn't trigger note, because typ.definition.fullname() returns None.

Contributor

miedzinski commented May 16, 2017

I have no idea how to add test case for this.

[case testUnexpectedKwargsDefinedHere]
import m
m.A(x=1)  # E: Unexpected keyword argument "x" for "A"
[file m.py]
class A:
    def __init__(self) -> None:  # N: "A" defined here
        pass

doesn't trigger note, because typ.definition.fullname() returns None.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi May 21, 2017

Collaborator

@miedzinski
I have found what is the problem. The _fulname attribute of function definition is not set in first pass for methods, since methods are not visited during the first pass. I added this code:

            if not defn._fullname:
                defn._fullname = self.qualified_name(defn.name())

in visit_func_def of second pass on line 330 and it works:

tst22.py:21: error: Unexpected keyword argument "example" for "Popen"
/usr/local/lib/mypy/typeshed/stdlib/3/subprocess.pyi:276: note: "Popen" defined here

(also works with your test example above).

Collaborator

ilevkivskyi commented May 21, 2017

@miedzinski
I have found what is the problem. The _fulname attribute of function definition is not set in first pass for methods, since methods are not visited during the first pass. I added this code:

            if not defn._fullname:
                defn._fullname = self.qualified_name(defn.name())

in visit_func_def of second pass on line 330 and it works:

tst22.py:21: error: Unexpected keyword argument "example" for "Popen"
/usr/local/lib/mypy/typeshed/stdlib/3/subprocess.pyi:276: note: "Popen" defined here

(also works with your test example above).

@miedzinski

This comment has been minimized.

Show comment
Hide comment
@miedzinski

miedzinski May 21, 2017

Contributor

Do you want me to include it with this patch or would you like to submit it yourself?

Contributor

miedzinski commented May 21, 2017

Do you want me to include it with this patch or would you like to submit it yourself?

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi May 21, 2017

Collaborator

Do you want me to include it with this patch

Yes.

Collaborator

ilevkivskyi commented May 21, 2017

Do you want me to include it with this patch

Yes.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi May 21, 2017

Collaborator

And tests

Collaborator

ilevkivskyi commented May 21, 2017

And tests

@ilevkivskyi

Thanks! Looks good to me.

@ilevkivskyi ilevkivskyi merged commit 48c6425 into python:master May 22, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum May 24, 2017

Member

I just received a spurious message of the form

/Users/guido/src/mypy/typeshed/stdlib/3/builtins.pyi:594: note: "sort" of "list" defined here

The code to issue this message was modified by this PR.

I have not been able to repro this "in the lab" yet (the above was part of a huge run on internal code), but I suspect that this might be related to an error in another file that was ignored by # type: ignore in some other file. The fix would be to somehow check whether the fail() call actually caused an error to be generated or not. (That would require some surgery to get the error machinery to return that information -- several methods would have to pass this info back reliably.)

Member

gvanrossum commented May 24, 2017

I just received a spurious message of the form

/Users/guido/src/mypy/typeshed/stdlib/3/builtins.pyi:594: note: "sort" of "list" defined here

The code to issue this message was modified by this PR.

I have not been able to repro this "in the lab" yet (the above was part of a huge run on internal code), but I suspect that this might be related to an error in another file that was ignored by # type: ignore in some other file. The fix would be to somehow check whether the fail() call actually caused an error to be generated or not. (That would require some surgery to get the error machinery to return that information -- several methods would have to pass this info back reliably.)

@pkch

This comment has been minimized.

Show comment
Hide comment
@pkch

pkch May 24, 2017

Contributor

I don't have a specific solution proposal yet, but a very similar issue came up in my discussions with @ilinum about how to do #3141 more reliably: it would have helped if the error engine was more knowledgeable.

Contributor

pkch commented May 24, 2017

I don't have a specific solution proposal yet, but a very similar issue came up in my discussions with @ilinum about how to do #3141 more reliably: it would have helped if the error engine was more knowledgeable.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum May 24, 2017

Member

I also noticed that the spurious note seems to only appear with a cold -i cache. This may be a different problem with the same code -- finding the module may fail if the module was read from the cache.

Member

gvanrossum commented May 24, 2017

I also noticed that the spurious note seems to only appear with a cold -i cache. This may be a different problem with the same code -- finding the module may fail if the module was read from the cache.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi May 24, 2017

Collaborator

I would propose an alternative solution. We could report the note on the same line as error, but report the source in the note itself, something like:

test.py: 16: error: Incompatible something ...
test.py: 16: note: "something" defined in /some/other/file on line 144

Adding # type: ignore will silence both the error and the note. This solution will have one important additional benefit: there is an issue to sort errors by file, and after sorting all errors, the error and the note may be far apart from each other, which would be weird. With this solution they will be always together.

(TBH, this wording also seems nicer to me than the current one.)

Collaborator

ilevkivskyi commented May 24, 2017

I would propose an alternative solution. We could report the note on the same line as error, but report the source in the note itself, something like:

test.py: 16: error: Incompatible something ...
test.py: 16: note: "something" defined in /some/other/file on line 144

Adding # type: ignore will silence both the error and the note. This solution will have one important additional benefit: there is an issue to sort errors by file, and after sorting all errors, the error and the note may be far apart from each other, which would be weird. With this solution they will be always together.

(TBH, this wording also seems nicer to me than the current one.)

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum May 24, 2017

Member

Finally it may be the case that what I saw was due to some filtering script we have that black-lists certain files. (But the -i issue is still separate -- even if this PR did not change the behavior here.)

Member

gvanrossum commented May 24, 2017

Finally it may be the case that what I saw was due to some filtering script we have that black-lists certain files. (But the -i issue is still separate -- even if this PR did not change the behavior here.)

@miedzinski

This comment has been minimized.

Show comment
Hide comment
@miedzinski

miedzinski May 24, 2017

Contributor

Already said that on gitter: this is might have been introduced in this patch, mainly because of fix @ilevkivskyi proposed. Earlier methods lacked fullnames during 2nd pass, so the note wasn't triggered.

Contributor

miedzinski commented May 24, 2017

Already said that on gitter: this is might have been introduced in this patch, mainly because of fix @ilevkivskyi proposed. Earlier methods lacked fullnames during 2nd pass, so the note wasn't triggered.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum May 24, 2017

Member

A downside to @ilevkivskyi's solution is that I can't hit Enter in the Emacs *compilation* window to go to the definition.

Member

gvanrossum commented May 24, 2017

A downside to @ilevkivskyi's solution is that I can't hit Enter in the Emacs *compilation* window to go to the definition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment