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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow line-level type controls #3143

Closed
jab opened this issue Mar 3, 2022 · 13 comments
Closed

Allow line-level type controls #3143

jab opened this issue Mar 3, 2022 · 13 comments
Labels
addressed in next version Issue is fixed and will appear in next published version enhancement request New feature or request

Comments

@jab
Copy link

jab commented Mar 3, 2022

Is your feature request related to a problem? Please describe.

To use pyright in my project, I need to be able to control it on a per-line basis, using a comment on the same line as a line of code, similar to what mypy supports. See https://github.com/jab/bidict/blob/7c736bfb8389734e5350827ae4386c4919ce606c/bidict/_frozenbidict.py#L38 for an example:

self._hash = t.ItemsView(self)._hash()  # type: ignore [attr-defined]  # https://github.com/python/typeshed/pull/7153

But pyright currently only supports file-level type controls. Ref: https://github.com/microsoft/pyright/blob/main/docs/comments.md#file-level-type-controls

This is a problem because

  1. sometimes you need to only ignore a particular type of error on a particular line, not for an entire file, as in the example above
  2. users coming from other type checkers where this is possible will reasonably expect pyright to be able to do this, and when they find out it can't, they are more likely to use an alternative type checker instead.

Describe the solution you'd like
When a pyright comment occurs on the same line as a line of code, it should only apply to that line, not the entire file:

# Per-file settings are still respected:
# pyright: strict, reportPrivateUsage=true

# But can be overridden on a per-line basis when necessary:
x = t.ItemsView(self)._hash()  # pyright: reportPrivateUsage=false

# Back at file-level...
foo._private = 42  # ...this still causes a privateUsage error

Additional context
"Just use cast() instead" is not always practical or desired, and I think will just contribute to driving more users to alternative type checkers.

Thank you for your work on pyright and for your consideration of this suggestion.

@jab jab added the enhancement request New feature or request label Mar 3, 2022
@erictraut
Copy link
Collaborator

This idea has been discussed before and rejected for a couple of reasons.

First, suppression of errors should generally be avoided. If you find yourself using error suppression in all but the rarest of circumstances, there is probably a better approach. If you're finding that you need to suppress many errors in your code, I'd be interested to understand what diagnostic check is creating all the noise. In your example above, you showed reportPrivateUsage. Is that the main culprit, or are there others?

Second, there's already a standard way defined in PEP 484 to suppress diagnostics on a per-line basis: # type: ignore. The standard doesn't allow for specifying individual diagnostic rules, and there is no standardization of diagnostic rule names anyway.

Third, it's exceedingly rare to have multiple diagnostics appear on a single line, so specifying which diagnostic rule you want to suppress on a per-line basis provides little or no utility.

@erictraut erictraut added the as designed Not a bug, working as intended label Mar 3, 2022
@jab
Copy link
Author

jab commented Mar 3, 2022

There are others. Here are the 'type: ignore' comments I've been using with mypy:

rg 'type: ignore' bidict
bidict/_iter.py
24:    yield from kw.items()  # type: ignore [misc]

bidict/_named.py
67:    class NamedBidict(base_type, NamedBidictBase):  # type: ignore [valid-type,misc]  # https://github.com/python/mypy/issues/5865

bidict/_frozenbidict.py
38:            self._hash = t.ItemsView(self)._hash()  # type: ignore [attr-defined]  # https://github.com/python/typeshed/pull/7153

bidict/_orderedbase.py
45:        return getattr(instance, self.slot)()  # type: ignore [no-any-return]
85:    nxt: WeakAttr['SentinelNode', Node] = WeakAttr(slot='_nxt_weak')  # type: ignore [assignment]

bidict/_base.py
531:        return self._from_other, (cls, dict(init_from), should_invert)  # type: ignore [call-overload] # https://github.com/python/mypy/issues/4975

When I turn on pyright type checking for my project, there appear to be a lot of bogus errors.

In https://github.com/jab/bidict there is a combination of factors at play that it sounds like you're not designing for:

  • It's a relatively small library (~1K SLOC).
  • Despite its small size, its core purpose necessarily hits up against longstanding limitations in Python's type system (e.g. Higher-Kinded TypeVars python/typing#548 (comment)), longstanding bugs in various type checkers (e.g. Function argument rejected as a base class python/mypy#5865), and probably design and investment tradeoffs made by type checker implementors to optimize for more common use cases.
  • Has only a single, voluntary maintainer (me) whose time is limited and who necessarily needs to prioritize more practical and expedient solutions sometimes. If a non-type: ignore solution to a bogus type error requires dramatically increasing the tooling noisiness of the code, i.e. more and more ink gets spilled appeasing various, confused linters than on solving the actual problem, such maintainers may reasonably choose to type: ignore and move on.

It seems I've hit another case of this, in which case, fair enough, but open to hearing if you think otherwise. Thanks again for your consideration.

@erictraut
Copy link
Collaborator

erictraut commented Mar 3, 2022

Thanks for the additional information.

I cloned your repo and took a look at it. (This is a good code base — nicely structured, easy to follow, well commented, and well typed.)

When I run pyright on this code base in "basic" type checking mode, I receive 17 errors.

  • 2 are already suppressed by your existing # type: ignore comments that you have in place for mypy
  • 1 was a bug in pyright (the fix will be in the next release)
  • 5 are in tests and appear to be intentional type errors to exercise error paths

Of the remaining 9, most of them appear to be due to small difference between behaviors in mypy and pyright. They can be addressed through small tweaks to type annotations and/or the addition of a few typing.cast calls.

I don't see anything that would requires explicit suppression of errors. I presume that addressing 9 issues isn't too much of a burden.

If you enable all of pyright's strictest checks, many more errors are generated (1041 to be exact). That would be a lot of work to address, but you could enable a subset of these stricter rules if you see value in them. I enabled all of the strictest checks except for four. This produced only 5 additional diagnostics, and they should be easy to address. For reference, here's what I added to your pyproject.toml:

[tool.pyright]
exclude = ["tests", "docs"]
enableTypeIgnoreComments = true,
strictListInference = true,
strictDictionaryInference = true,
strictParameterNoneValue = true,
reportFunctionMemberAccess = "error"
reportMissingTypeStubs = "error"
reportUnusedImport = "error"
reportUnusedClass = "error"
reportUnusedFunction = "error"
reportUnusedVariable = "error"
reportDuplicateImport = "error"
reportUntypedFunctionDecorator = "error"
reportUntypedClassDecorator = "error"
reportUntypedBaseClass = "error"
reportUntypedNamedTuple = "error"
reportConstantRedefinition = "error"
reportIncompatibleMethodOverride = "error"
reportIncompatibleVariableOverride = "error"
reportInvalidStringEscapeSequence = "error"
reportUnknownParameterType = "error"
reportUnknownLambdaType = "error"
reportMissingTypeArgument = "error"
reportInvalidStubStatement = "error"
reportInvalidTypeVarUse = "error"
reportPropertyTypeMismatch = "error"
reportSelfClsParameterName = "error"
reportOverlappingOverload = "error"
reportUnsupportedDunderAll = "error"
reportUnusedCallResult = "error"
reportMatchNotExhaustive = "error"

# Leave these off for now
reportPrivateUsage = "none"
reportUnknownArgumentType = "none"
reportUnknownVariableType = "none"
reportUnknownMemberType = "none"

I'll think more about adding line-level suppression of specific diagnostics, but I don't think this code base provides a strong justification for this feature.

@erictraut erictraut added needs decision and removed as designed Not a bug, working as intended labels Mar 4, 2022
@erictraut
Copy link
Collaborator

I've thought about this more, and I don't think it makes sense to implement this feature at this time. If I were to implement it, I'd probably use the same mechanism as mypy — i.e. a comma-delimited list of diagnostic rules in square brackets after the # type: ignore comment. But since pyright uses different diagnostic rule names (and a different breakdown of diagnostics) than mypy, users would need to list both mypy and pyright diagnostic rules if they want to use both type checkers on the same code base. Also, it's not clear what mypy or pyright would do if they see a set of diagnostic rule names and don't recognize any of them. Should they treat that as a regular # type: ignore? Or should they treat it as a # type: ignore that applies to no diagnostic rules? Either choice can result in problems — false positives and false negatives.

For now, we'll stick with the documented support for # type: ignore and ignore anything after the ignore.

@jab
Copy link
Author

jab commented Mar 10, 2022

Followup on some minor points I didn't respond to yet (the second half of this comment is more important for now, but just to get this out of the way first):

This idea has been discussed before and rejected for a couple of reasons.

(I searched the tracker before opening this but didn't find previous discussion of this precise suggestion, sorry if I missed it! If you have any links handy, I'd still love to catch up on any discussion I missed.)

it's exceedingly rare to have multiple diagnostics appear on a single line, so specifying which diagnostic rule you want to suppress on a per-line basis provides little or no utility.

I've found more than a little utility in being able to suppress specific rules on a per-line basis with mypy. My experience with mypy has been that it's changed and improved over time, often requiring me to change or remove my existing "type: ignore [rule]" comments when I've upgraded. Being able to see and control the exact scope of what's being suppressed right in the code has made this upgrade work easier.

1 was a bug in pyright (the fix will be in the next release)

On reflection, I wonder if it's worth calling out whether this points at one of the strongest arguments in favor of implementing either this suggestion or some other affordance. When there's a bug in a type checker, a precisely-targeted "type: ignore" comment will often be the most maintainable way that users can work around the bug (until such time as it's fixed).


Just got a chance to follow up on this. Honored you took the time to test pyright so carefully with bidict and share your results (and even a suggested pyright config!), to read bidict's code and give feedback (your kind words meant a lot, thank you!), and to give this suggestion more thought. I understand if you're convinced this is not the right way forward at this time.

I was inspired by your writeup to put more time into using pyright with bidict. Testing further with your suggested config, I found a few opportunities to make positive changes to bidict's code and/or type hints based on some of the errors pyright was emitting.

In other cases, I didn't have as much luck. I've consolidated the remaining issues using pyright with bidict in jab/bidict#242, and hope the review questions/comments that I left there will uncover more opportunities to improve pyright and/or mypy and/or bidict.

If you get a chance to take a look at that PR, I'd appreciate your thoughts, as they relate back to this issue and any others.

Thanks so much again!

@erictraut
Copy link
Collaborator

Thanks @jab. I'll take a look a the PR later today.

@erictraut
Copy link
Collaborator

After much discussion, we've decided to add support for a # pyright: ignore comment. This also supports rule-specific suppression, as in # pyright: ignore [reportUnusedImport, reportGeneralTypeIssues]. For more details, refer to this documentation. This will be included in the next release.

@erictraut erictraut reopened this Mar 11, 2022
@erictraut erictraut added the addressed in next version Issue is fixed and will appear in next published version label Mar 11, 2022
@jab
Copy link
Author

jab commented Mar 11, 2022

Great news, thanks! This will make it so much easier for bidict to adopt pyright (and enable pyright type checking by default in its devcontainer VS Code setup), and I hope it helps more projects adopt pyright more quickly too. Thanks so much for reconsidering this!

@jab
Copy link
Author

jab commented Mar 11, 2022

Read through the https://github.com/microsoft/pyright/blob/main/docs/comments.md#line-level-diagnostic-suppression docs. This is fantastic.

pyright does not yet support opting into warnings or errors for any unused suppression comments, is that right? If so, would you be willing to consider that as well? That feature is the other side of the coin for the use case of working around a pyright bug via line-level suppression comment, then upgrading to a new pyright version where the bug is fixed. Another feature that users coming to pyright from mypy may miss: https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-warn-unused-ignores

@erictraut
Copy link
Collaborator

Pyright already supports this feature. You can enable "reportUnnecessaryTypeIgnoreComment".

@jab
Copy link
Author

jab commented Mar 11, 2022

Sorry I missed that and great to hear!

Worth mentioning this in the https://github.com/microsoft/pyright/blob/main/docs/comments.md#line-level-diagnostic-suppression and/or https://github.com/microsoft/pyright/blob/main/docs/configuration.md docs?

I'm thinking of prior art like https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-warn-unused-ignores

This flag, along with the --warn-redundant-casts flag, are both particularly useful when you are upgrading mypy. Previously, you may have needed to add casts or # type: ignore annotations to work around bugs in mypy or missing stubs for 3rd party libraries.

These two flags let you discover cases where either workarounds are no longer necessary.

@erictraut
Copy link
Collaborator

Yeah, that's a good suggestion. I just updated the docs.

@erictraut
Copy link
Collaborator

This is addressed in pyright 1.1.229, which I just published. It will also be included in the next release of pylance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addressed in next version Issue is fixed and will appear in next published version enhancement request New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants