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

FBT001 preview behavior may lead to false positives #14444

Open
dylwil3 opened this issue Nov 18, 2024 · 2 comments
Open

FBT001 preview behavior may lead to false positives #14444

dylwil3 opened this issue Nov 18, 2024 · 2 comments
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Milestone

Comments

@dylwil3
Copy link
Collaborator

dylwil3 commented Nov 18, 2024

The rule boolean-type-hint-positional-argument (FBT001) was extended in #7501 , under preview, to also flag positional arguments that have a type hint with any union of types that includes bool.

The intent of the rules is specifically to find places where a boolean is used as a (binary) flag, in a positional argument. If an argument has a type hint of bool or even bool | None, it makes sense to predict that the argument is being used as a flag. But the preview behavior allows any union, even, say bool | str | MyClass. This seems like a situation where it's a bit of a stretch to assume that the user is using the boolean as a flag.

Maybe the concern is unfounded and this doesn't actually happen in the real world, but I figured the community could weigh in a bit on the point before stabilizing this preview behavior.

@dylwil3 dylwil3 added the preview Related to preview mode features label Nov 18, 2024
@dylwil3 dylwil3 added this to the v0.9 milestone Nov 18, 2024
@dhruvmanila dhruvmanila added the rule Implementing or modifying a lint rule label Nov 19, 2024
@MichaReiser
Copy link
Member

I'm just curious. Do you have any examples from the ecosystem checks or from real world projects that raised this concern?

@dylwil3
Copy link
Collaborator Author

dylwil3 commented Nov 23, 2024

It's rare but it does happen - but maybe rare enough that noqa is the answer. Here is an example from pandas:

https://github.com/pandas-dev/pandas/blob/ee0902a832b7fa3e5821ada176566301791e09ec/pandas/io/formats/info.py#L350-L356

and a strange one from Pytorch:

https://github.com/pytorch/pytorch/blob/3473dfa69859f4049abe46075a91014922af198c/torch/onnx/_internal/exporter/_torchlib/ops/hop.py#L13-L19

It's relatively common for there to be a type alias for scalars that includes bool along with float etc., but the rule wouldn't flag that...

All that to say, it's probably rare enough that I'm being overly cautious here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

No branches or pull requests

3 participants