-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Comments
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 Second, there's already a standard way defined in PEP 484 to suppress diagnostics on a per-line basis: 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. |
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 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. |
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.
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 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:
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. |
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 For now, we'll stick with the documented support for |
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):
(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.)
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.
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! |
Thanks @jab. I'll take a look a the PR later today. |
After much discussion, we've decided to add support for a |
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! |
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 |
Pyright already supports this feature. You can enable "reportUnnecessaryTypeIgnoreComment". |
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
|
Yeah, that's a good suggestion. I just updated the docs. |
This is addressed in pyright 1.1.229, which I just published. It will also be included in the next release of pylance. |
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:
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
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:
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.
The text was updated successfully, but these errors were encountered: