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 serializing a type-ignored property setter #3493

Merged
merged 3 commits into from Jun 3, 2017

Conversation

Projects
None yet
2 participants
@JelleZijlstra
Collaborator

JelleZijlstra commented Jun 2, 2017

This fixes two issues from #3491: a crash in serialization and an incorrect error "Function is missing a type annotation for one or more arguments". Both had the same root cause; we were skipping semantic analysis for the function body if mypy was unhappy with the property.setter decorator.

fix crash when serializing a type-ignored property setter
This fixes the crash from #3491. I wasn't able to reproduce the
crash in a test, even with '# flags: --incremental', so I'm
not adding any new tests. Do I need some special flags to make
tests write the cache?
@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jun 2, 2017

Member

I think only tests in check-incremental.test and tests whose name ends in 'Incremental' exercise serialization.

Member

gvanrossum commented Jun 2, 2017

I think only tests in check-incremental.test and tests whose name ends in 'Incremental' exercise serialization.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jun 2, 2017

Member

So please add such a test.

Member

gvanrossum commented Jun 2, 2017

So please add such a test.

Add tests
Turns out the cache isn't written if there is any error,
and I had a reveal_type which counts as an error.

The change fixes both bugs from the original issue:
- Without --disallow-untyped-defs, it no longer crashes during
  cache serialization.
- With --disallow-untyped-defs, it no longer produces a spurious
  error because "self" doesn't have an annotation.
@JelleZijlstra

This comment has been minimized.

Show comment
Hide comment
@JelleZijlstra

JelleZijlstra Jun 3, 2017

Collaborator

Thanks, it turns out the issue was that the cache isn't written when there is an error in the file, and I had a reveal_type(), which counts as an error.

In other news, it turns out that this PR also fixes the other bug reported in #3491 (the spurious "missing type annotation" error). Therefore, I added tests both with and without --disallow-untyped-defs.

Collaborator

JelleZijlstra commented Jun 3, 2017

Thanks, it turns out the issue was that the cache isn't written when there is an error in the file, and I had a reveal_type(), which counts as an error.

In other news, it turns out that this PR also fixes the other bug reported in #3491 (the spurious "missing type annotation" error). Therefore, I added tests both with and without --disallow-untyped-defs.

@@ -564,9 +564,10 @@ def analyze_property_with_multi_part_definition(self, defn: OverloadedFuncDef) -
first_item.var.is_settable_property = True
# Get abstractness from the original definition.
item.func.is_abstract = first_item.func.is_abstract
item.func.accept(self)
else:
self.fail("Decorated property not supported", item)

This comment has been minimized.

@gvanrossum

gvanrossum Jun 3, 2017

Member

This should also be fixed (it's in a sense the true reason for the issue). But not in this PR.

@gvanrossum

gvanrossum Jun 3, 2017

Member

This should also be fixed (it's in a sense the true reason for the issue). But not in this PR.

@gvanrossum

Great work. I'll merge once tests pass again after my PEP 8 change.

@gvanrossum gvanrossum merged commit b5fa5d2 into python:master Jun 3, 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 Jun 3, 2017

Member

Oh, sorry, I should have copied your updated PR description. Oh well.

Member

gvanrossum commented Jun 3, 2017

Oh, sorry, I should have copied your updated PR description. Oh well.

@JelleZijlstra JelleZijlstra deleted the JelleZijlstra:fix3491 branch Jun 3, 2017

carljm added a commit to carljm/mypy that referenced this pull request Jun 3, 2017

Merge branch 'master' into module-alias
* master:
  fix crash when serializing a type-ignored property setter (#3493)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment