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 config file parsing of disallow_any/disallow_untyped_defs combination #4076

Merged
merged 4 commits into from Oct 13, 2017

Conversation

Projects
None yet
5 participants
@JelleZijlstra
Collaborator

JelleZijlstra commented Oct 9, 2017

Fixes #4075.

Due to the way disallow_any is parsed in config files, any disallow_any directive basically overrode disallow_untyped_defs. This confused me in #4075, and it also meant that disallow_untyped_defs was implicitly disabled for mypy itself, because of the way mypy_self_check.ini was written.

This PR solves the problem by making disallow_any override disallow_untyped_defs only if the config file either has disallow_any = (disabling all disallow_any checks) or explicitly lists unannotated (enabling disallow_untyped_defs). This passes all tests (including a new one), but it's awkward because it makes unannotated behave differently from other disallow_any flags: if a more general config has disallow_any = A B and a more specific one has disallow_any = A, then B should be disabled, but this is no longer true for unannotated.

Here are two other options for solving the problem:

  • We could parse disallow_untyped_defs and disallow_any completely independently, and just AND them at runtime when we check whether to give an error for an untyped def. This leads to an awkward situation because now if you have both disallow_any = unannotated in a general config and disallow_untyped_defs = False in the specific one, untyped defs still produce errors.
  • The root cause of the problem is that we have two options that do the same thing, so we should get rid of one of them so we don't have to worry about how they interact. I think that's what @ilinum originally did, but I argued against it because disallow_untyped_defs is more intuitively named. What if we just get rid of disallow_any = unannotated instead?

In this PR I also fix the untyped defs that crept into mypy itself due to the bug.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Oct 9, 2017

Member

Thanks for finding this. I have to agree that I would rather get rid of one of the two spellings (your second bullet). I also dislike the double negative of "disallow_any = unannotated" so I would prefer to get rid of that.

FWIW I also would prefer to have a bunch of separate Boolean flags instead of the single complex flag that is disallow_any. I argued against the current set-based design during its inception but lost; IIRC @ilinum eventually found a problem that would have been easier with the separate-flags design, but he was too far into the implementation to switch.

(That problem, FWIW, is that if you want to have disallow_any = X, Y, Z in your main section and then disable Y in a module-specific section, you have to write disallow_any = X, Z in that subsection. And then later if you change your main section to disallow_any = X, Y, Z, U you have to remember to add U to all the module-specific sections that override it (assuming they just meant to remove Y).)

Member

gvanrossum commented Oct 9, 2017

Thanks for finding this. I have to agree that I would rather get rid of one of the two spellings (your second bullet). I also dislike the double negative of "disallow_any = unannotated" so I would prefer to get rid of that.

FWIW I also would prefer to have a bunch of separate Boolean flags instead of the single complex flag that is disallow_any. I argued against the current set-based design during its inception but lost; IIRC @ilinum eventually found a problem that would have been easier with the separate-flags design, but he was too far into the implementation to switch.

(That problem, FWIW, is that if you want to have disallow_any = X, Y, Z in your main section and then disable Y in a module-specific section, you have to write disallow_any = X, Z in that subsection. And then later if you change your main section to disallow_any = X, Y, Z, U you have to remember to add U to all the module-specific sections that override it (assuming they just meant to remove Y).)

@JelleZijlstra

This comment has been minimized.

Show comment
Hide comment
@JelleZijlstra

JelleZijlstra Oct 9, 2017

Collaborator

Let's do this for now:

  • Remove disallow_any = unannotated in this PR, leaving only disallow_untyped_defs = True
  • File an issue to replace the other disallow_any flags with boolean flags. (I might be able to work on that, but not immediately.)

Does that sound good?

Collaborator

JelleZijlstra commented Oct 9, 2017

Let's do this for now:

  • Remove disallow_any = unannotated in this PR, leaving only disallow_untyped_defs = True
  • File an issue to replace the other disallow_any flags with boolean flags. (I might be able to work on that, but not immediately.)

Does that sound good?

@ilinum

This comment has been minimized.

Show comment
Hide comment
@ilinum

ilinum Oct 9, 2017

Collaborator

That sounds good to me!
Removing disallow_any=unannotated seems like a good call. Do we need to deprecate it first? Or can we just remove it? What's the right approach here?

I am also okay with replacing disallow_any with boolean flags for the reasons that Guido pointed out.

Collaborator

ilinum commented Oct 9, 2017

That sounds good to me!
Removing disallow_any=unannotated seems like a good call. Do we need to deprecate it first? Or can we just remove it? What's the right approach here?

I am also okay with replacing disallow_any with boolean flags for the reasons that Guido pointed out.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Oct 9, 2017

Member

I propose to just drop it. People who use it will get failures after they update (possibly at 0.540 already, which will be frozen this Friday and released Wednesday next week) but IMO that's fine, people always test new releases somewhat when they upgrade.

The only thing we might add would be to have a special case in the error message that checks if unannotated is among the given strings and then prints use --disallow-untyped-defs instead.

Member

gvanrossum commented Oct 9, 2017

I propose to just drop it. People who use it will get failures after they update (possibly at 0.540 already, which will be frozen this Friday and released Wednesday next week) but IMO that's fine, people always test new releases somewhat when they upgrade.

The only thing we might add would be to have a special case in the error message that checks if unannotated is among the given strings and then prints use --disallow-untyped-defs instead.

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Oct 13, 2017

Collaborator

The PR has merge conflicts.

Collaborator

JukkaL commented Oct 13, 2017

The PR has merge conflicts.

@JelleZijlstra

This comment has been minimized.

Show comment
Hide comment
@JelleZijlstra

JelleZijlstra Oct 13, 2017

Collaborator

Fixed.

Collaborator

JelleZijlstra commented Oct 13, 2017

Fixed.

@gvanrossum gvanrossum merged commit 47e53f8 into python:master Oct 13, 2017

2 checks passed

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

@JelleZijlstra JelleZijlstra deleted the JelleZijlstra:issue4075 branch Oct 13, 2017

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