Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignoring a file with a single # type: ignore comment. #6830

Merged
merged 7 commits into from May 16, 2019

Conversation

@brandtbucher
Copy link
Contributor

commented May 14, 2019

Fixes #626. For background on the design choices, here is what our current ASTs allow us to find:

  • In all versions of Python, we can accurately detect bare # type: ignore comments before the first statement in a module.
  • In Python 3.8+, we can accurately detect the placement of bare # type: ignore comments within a list of statements (we need end_lineno to do this properly).
  • In no current version of Python can we accurately detect bare # type: ignore comments before the first statement in an indented code block using the AST alone.

Considering these limitations, the behavior I chose here is:

  • All versions of Python will ignore a whole file if it contains a bare # type: ignore comment before its first statement.
  • Python 3.8+ will ignore all statements following a bare # type: ignore comment at the top level of a module. This is now a separate PR: #6841.
  • Bare # type: ignore comments have no special meaning inside of indented code blocks. If we were to allow this, we would need to do one of the following (and I personally don't like any of them):
    • Not recognize them at the start of a block. 馃檨
    • Guess where the for/while/with/class/def/try bit ends, a la decorated definitions prior to Python 3.8. 馃檨
    • Parse the raw source of the file ourselves to see where the above block-starters end. 馃檨
    • Modify ast parsing to tell us where the block-starters end. 馃檨

A few notes:

  • Ignoring is accomplished by nesting all remaining statements in an unreachable Block.
  • ASTConverter.extra_type_ignores: List[int] is now ASTConverter.type_ignores: Set[int] and is populated before the AST is converted.
  • Tests are added in check-ignore.test and check-38.test.
@brandtbucher

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

This only partially satisfies PEP 484's spec, but I still think it's a good start:

A # type: ignore comment on a line by itself is equivalent to adding an inline # type: ignore to each line until the end of the current indented block. At top indentation level this has effect of disabling type checking until the end of file.

In my opinion, the above wording is a bit too general. For one, it technically allows placing these ignores in weird places, like blank lines in the middle of expressions, or between a decorator and a function/class def. Taken literally, it also makes --warn-unused-ignores behavior super annoying, since every single line in the block now has a virtual # type: ignore comment.

@gvanrossum
Copy link
Member

left a comment

Great work! I have some suggestions but I think this is on the short road to success.

Could you add docs as well? Perhaps add something about ignoring a whole file somewhere in common_issues.rst.

return n.lineno

def translate_stmt_list(self,
l: Sequence[ast3.stmt],

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum May 14, 2019

Member

Please choose a longer name for this argument, maybe stmts? (I know the original code uses one-letter names, but I don't want to encourage the practice for new code, and you've added a lot here.)


if module and l and self.type_ignores and min(self.type_ignores) < self.get_line(l[0]):
self.errors.used_ignored_lines[self.errors.file].add(min(self.type_ignores))
b = Block(self.fix_function_overloads(self.translate_stmt_list(l)))

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum May 14, 2019

Member

Maybe rename to block?


def translate_stmt_list(self,
l: Sequence[ast3.stmt],
module: bool = False) -> List[Statement]:

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum May 14, 2019

Member

Perhaps rename to ismodule?


for i, e in enumerate(l):

if module: # and...

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum May 14, 2019

Member

Somehow it looks inefficient to check for module and sys.version_info each time through the loop. Also the comment here adds nothing. You did know and short-circuits, right? You can combine the two checks and save one level of indentation.

This comment has been minimized.

Copy link
@brandtbucher

brandtbucher May 14, 2019

Author Contributor

It was originally if module and sys.version_info >= (3, 8):, but the mypy self check complained when I accessed e.end_lineno on 336. As I understand it, the mypy's version checks don't work for compound tests like this? Splitting it up silences the error, and I believe the emitted bytecode is identical.

If it would be clearer, I can just # type: ignore line 336 instead!

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum May 14, 2019

Member

OK, then the two if statements is optimal -- just put in a clearer comment explaining that this is to work around a mypy issue.

for e in l:
line = 0

for i, e in enumerate(l):

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum May 14, 2019

Member

Again, I'd propose a longer name for e, e.g. stmt. (But, i for the index is traditional, so please keep that.)

if sys.version_info >= (3, 8):

# In Python 3.8, a "# type: ignore" comment between statements at
# the top level of a module skips checking for everything else:

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum May 14, 2019

Member

Most importantly, explain why: this check needs access to end_lineno which only exists in 3.8 and up.

def translate_stmt_list(self,
l: Sequence[ast3.stmt],
module: bool = False) -> List[Statement]:

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum May 14, 2019

Member

I'd leave this blank line out, as well a the one below this block comment. (Ditto for other block comments below; in general you sprinkle more whitespace throughout the code than is mypy's tradition.)

This comment has been minimized.

Copy link
@brandtbucher

brandtbucher May 14, 2019

Author Contributor

I've removed a lot of my extra whitespace throughout.

@@ -294,11 +294,50 @@ def translate_expr_list(self, l: Sequence[AST]) -> List[Expression]:
res.append(exp)
return res

def translate_stmt_list(self, l: Sequence[AST]) -> List[Statement]:
def get_line(self, n: ast3.stmt) -> int:

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum May 14, 2019

Member

Maybe rename to get_lineno? Without context, the name get_line vaguely makes me think that it would get the source code line somehow.

return MypyFile(body,
self.imports,
False,
set(ignores),
set(self.type_ignores),

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum May 14, 2019

Member

I'd add a comment explaining why it's copied here. (If there is a good reason!)

This comment has been minimized.

Copy link
@brandtbucher

brandtbucher May 14, 2019

Author Contributor

Looks like this isn't needed. Removed.

@brandtbucher

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

@gvanrossum I've added docs and addressed your concerns. Tests are passing, ready for another review!

@gvanrossum
Copy link
Member

left a comment

Almost there! I just wish the doc section to be moved a bit.

@@ -131,6 +131,30 @@ The second line is now fine, since the ignore comment causes the name
if we did have a stub available for ``frobnicate`` then mypy would
ignore the ``# type: ignore`` comment and typecheck the stub as usual.

A ``# type: ignore`` comment at the top of a module (before any statements,

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum May 15, 2019

Member

This hardly qualifies as "locally" silencing the checker, so I think it doesn't belong in this section.

I would make it a new section titled "Ignoring a whole file" to be inserted after this section (i.e. just before the section named "Unexpected errors about 'None' and/or 'Optional' types").

This comment has been minimized.

Copy link
@brandtbucher

brandtbucher May 15, 2019

Author Contributor

I've broken it out, as requested.

import frobnicate
frobnicate.start()
When running mypy with Python 3.8 or later, a ``# type: ignore`` comment

This comment has been minimized.

Copy link
@JelleZijlstra

JelleZijlstra May 15, 2019

Collaborator

This doesn't seem like a very useful behavior to me; when would you actually want this?

It's also potentially error-prone. In the example below, a user could write the # type: ignore expecting to silence errors on the import frobnicate line, but then get confused when this actually silences all errors in the whole file.

This comment has been minimized.

Copy link
@JelleZijlstra

JelleZijlstra May 15, 2019

Collaborator

For context, pyre's behavior is for an ignore comment on a line by itself to suppress errors on only the next line: https://pyre-check.org/docs/error-suppression.html#explicitly-suppressing-errors.

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum May 15, 2019

Member

expecting to silence errors on the import frobnicate line

Why would you expect that to work?

Also, the behavior implemented here is specified by PEP 484.

This comment has been minimized.

Copy link
@brandtbucher

brandtbucher May 15, 2019

Author Contributor

I had assumed that there was some degree of consensus between discussion on #626 and the relevant section of PEP 484 that a # type: ignore on its own line should disable type checking "until the end of the current indented block". This PR is an attempt to bring mypy closer to that spec. I don't have a personal use case for it, but I assume the PEP's authors gave it some thought? Maybe it's worth adding a note or some other warning to the user to help avoid mistakes.

One scenario I can imagine is somebody working their way through an existing code base with several large modules, adding or correcting annotations as they go. Test code also comes to mind, where you may have several fixtures you want type-checked near the top of the module, but intentionally broken code after that.

If we decide that the ignore-rest-of-file-on-3.8 part of this PR should be removed or deferred, I'm fine with that, though. I added it because I could for 3.8, but I'm certainly not married to it.

This comment has been minimized.

Copy link
@JelleZijlstra

JelleZijlstra May 15, 2019

Collaborator

Why would you expect that to work?

It's the intuitively more obvious behavior to me (which, granted, is not a very strong argument), and it's also what Pyre implemented. On the other hand, pytype's behavior is closer to what PEP 484 specifies, but with a way to turn type checking back on: https://github.com/google/pytype/blob/master/docs/user_guide.md#silencing-errors.

Also, the behavior implemented here is specified by PEP 484.

That's true, but this is a part of PEP 484 that has remained unimplemented in mypy for five years. As far as I can tell there have been few if any user requests for the full behavior specified in PEP 484鈥攗sers want to be able to ignore the whole file, but I don't see people asking for "ignore errors from an arbitrary point to the end of the file". So I think it's worth asking if the PEP 484 behavior really is what we want. We can always change PEP 484 too.

If #2938 is implemented, even using # type: ignore to ignore the whole file could be a redundant feature.

This comment has been minimized.

Copy link
@JelleZijlstra

JelleZijlstra May 15, 2019

Collaborator

Also, I'm happy to take this to the python/typing tracker to discuss potentially changing the PEP. My concern on this PR is that we don't introduce this behavior into mypy too soon and produce backward compatibility problems if we ever decide to change it.

@gvanrossum

This comment has been minimized.

Copy link
Member

commented May 16, 2019

I'm surprised this is so controversial, but given that it is, I am okay with taking it to a larger forum (though I have a slight preference for typing-sig, which probably reaches more people).

Note that there is a specific use case that depends on this which we use heavily at Dropbox: a # type: ignore at the very end of a file has no effect on the type checker. (At Dropbox, it results in our internal wrapper script adding the file to the mypy "build" even if it has no other type comments.)

I agree that there's probably no strong use case for putting a # type: ignore in the "middle" of a file.

I don't think that #2938 covers this, since that is mypy-specific; the "lone type-ignore" syntax is meant to be honored by all type checkers (and I feel quite apologetic that it has taken mypy so long to implement it).

@brandtbucher

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Thanks for the honest feedback @JelleZijlstra. I agree that something that seems as error prone as this deserves to be thoughtfully considered.

@gvanrossum Should I break out the partial-file behavior into another PR, and leave the whole-file behavior here? It seems like that specific feature is useful and uncontroversial by itself (it鈥檚 also been requested enough to be marked as 鈥渉igh priority鈥 for several years).

@gvanrossum

This comment has been minimized.

Copy link
Member

commented May 16, 2019

@JelleZijlstra

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2019

Yes, I don't have any problem with the behavior where # type: ignore at the top of the file ignores the whole file.

@brandtbucher brandtbucher changed the title Ignoring all or part of a file with a single # type: ignore comment. Ignoring a file with a single # type: ignore comment. May 16, 2019

@brandtbucher

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

@gvanrossum This branch is clean now!

@gvanrossum gvanrossum merged commit c73c95c into python:master May 16, 2019

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.

Copy link
Member

commented May 16, 2019

OK! Jelle, please start a conversation on whether the language about this in PEP 484 should be changed.

@brandtbucher brandtbucher deleted the brandtbucher:ignore-file branch May 16, 2019

PattenR added a commit to PattenR/mypy that referenced this pull request Jun 23, 2019

Ignoring a file with a single # type: ignore comment. (python#6830)
Fixes python#626

(There are potential further features as discussed in the PR, but these are somewhat controversial and may even be excised from PEP 484.)
@lazka lazka referenced this pull request Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.