DetachHead / basedpyright

pyright fork with various type checking improvements, improved vscode support and pylance features built into the language server
https://docs.basedpyright.com
Other
869 stars 16 forks source link

`reportUnnecessaryTypeIgnoreComment` false positive with method overloading #496

Closed injust closed 1 month ago

injust commented 1 month ago

With this code snippet:

from typing import overload

from attrs import define

@define(init=True)
class Foo:
    _bar_or_baz: str | int

    @overload  # type: ignore[no-overload-impl]
    def __init__(self, bar: str) -> None: ...  # pyright: ignore[reportNoOverloadImplementation]

    @overload
    def __init__(self, baz: int) -> None: ...

The ignore comments are required because mypy and basedpyright do not know about the attrs-generated __init__.

However, # type: ignore[no-overload-impl] causes basedpyright to report a reportUnnecessaryTypeIgnoreComment error.

I can't figure out how to ignore the error, either. If I change the line to:

@overload  # type: ignore[no-overload-impl]  # pyright: ignore[reportUnnecessaryTypeIgnoreComment]

Now there are TWO reportUnnecessaryTypeIgnoreComment errors.

DetachHead commented 1 month ago

use pyright: ignore comments instead, and set enableTypeIgnoreComments to false. basedpyright will soon do this by default due to issue like this. more info here: https://github.com/DetachHead/basedpyright/issues/330#issuecomment-2084382899

injust commented 1 month ago

That's a mypy ignore comment, not pyright.

As it stands, it's not possible to make this code pass with both mypy and basedpyright.

DetachHead commented 1 month ago

yeah, the issue is that basedpyright is looking at the type: ignore[no-overload-impl] comment when it's only intended for mypy right? setting enableTypeIgnoreComments to false will make basedpyright ignore all type: ignore comments. then you just have to change all type: ignore comments that are intended for basedpyright to pyright: ignore instead

injust commented 1 month ago

Ah sorry, I didn't see your edit.

enableTypeIgnoreComments = false would solve this. But I'd need to duplicate any existing # type: ignore[arg-type] comments into # pyright: ignore[...]. That sounds ergonomically painful, especially when it's inline comments.

When using mypy and pyright together on a codebase (I'm not sure how common this is), it feels easier to set enableTypeIgnoreComments = true and reportUnnecessaryTypeIgnoreComment = false.

So maybe it's better to make # pyright: ignore[reportUnnecessaryTypeIgnoreComment] work. The most reasonable alternative to that is disabling the check completely, which is objectively worse.

KotlinIsland commented 1 month ago

unrelated to this general problem, this specific problem would be resolved if these type only defs were in a TYPE_CHECKING block. Then this false error wouldn't be reported to begin with

unfortunately this behaviour only exists in basedmypy at the moment

KotlinIsland commented 1 month ago

i agree, it would be good to see:

a  # type: ignore[reportUndefindVariable, name-defined]

work with mypy and pyright together. they could just ignore any codes that they don't recognise I suppose. it would be a config option of course, because not everyone uses multiple type checkers

DetachHead commented 1 month ago

enableTypeIgnoreComments = false would solve this. But I'd need to duplicate any existing # type: ignore[arg-type] comments into # pyright: ignore[...]. That sounds ergonomically painful, especially when it's inline comments.

doing that is preferred imo because if you only use the type: ignore comment, pyright will ignore any error regardless of its diagnostic code, even if it's a completely invalid code that doesn't exist in either type checker:

def foo(value: int): ...

# no error here, even though asdf is not a rule recognized by pyright
foo("") # type:ignore[asdf]

this is the only reason your type:ignore comments are working on both mypy and pyright. but it's safer to explicitly specify the error code to both type checkers, even if it's annoying to do so.

When using mypy and pyright together on a codebase (I'm not sure how common this is), it feels easier to set enableTypeIgnoreComments = true and reportUnnecessaryTypeIgnoreComment = false.

i have a codebase that uses both basedmypy and basedpyright so it is a supported use case. though out of curiosity, is this because you're developing a library that needs to be compatible with both mypy and pyright, or some other reason?

So maybe it's better to make # pyright: ignore[reportUnnecessaryTypeIgnoreComment] work. The most reasonable alternative to that is disabling the check completely, which is objectively worse.

although i agree that should work, i don't think it would really help here, because typing this is equally annoying as having to use both type:ignore and pyright:ignore comments with each error code, only less safe:

@overload  # type: ignore[no-overload-impl]  # pyright: ignore[reportUnnecessaryTypeIgnoreComment]
injust commented 1 month ago

i have a codebase that uses both basedmypy and basedpyright so it is a supported use case. though out of curiosity, is this because you're developing a library that needs to be compatible with both mypy and pyright, or some other reason?

Not a library. Mainly because mypy and pyright both catch things that the other misses.

I started out using mypy only, but pyright (or at least its VS Code integration) seems significantly faster.

Also because VS Code's mypy integration is terrible and the bundled dmypy was broken for many months. Even now, they ship mypy 1.6.1, which is 9 months old.

DetachHead commented 1 month ago

Even now, it ships with mypy 1.6.1, which is 9 months old.

you can fix that by setting mypy-type-checker.importStrategy to "fromEnvironment" to make it use the version of mypy you have installed (i have no idea why "useBundled" is the default, it should just fall back to that if you don't have mypy installed in your project already)

injust commented 1 month ago

you can fix that by setting mypy-type-checker.importStrategy to "fromEnvironment" to make it use the version of mypy you have installed

Yeah I ended up doing that. Didn't want a venv for random scripts, but oh well.

(i have no idea why "useBundled" is the default, it should just fall back to that if you don't have mypy installed in your project already)

The fallback is broken 😆 https://github.com/microsoft/vscode-mypy/issues/184