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

Support an allowlist of function calls in argument defaults for B008 #3762

Closed
rouge8 opened this issue Mar 27, 2023 · 15 comments
Closed

Support an allowlist of function calls in argument defaults for B008 #3762

rouge8 opened this issue Mar 27, 2023 · 15 comments
Labels
accepted Ready for implementation good first issue Good for newcomers rule Implementing or modifying a lint rule

Comments

@rouge8
Copy link
Contributor

rouge8 commented Mar 27, 2023

It would be great if B008 supported an allowlist of function calls, similar to extend-immutable-calls for B006. We do a lot of this in our code which as far as I know has no subtle bugs:

def f(x=Decimal("1000")):
    pass

Or maybe it's worth hardcoding Decimal as an exception?

@charliermarsh
Copy link
Member

Yeah perhaps we add that to the list of IMMUTABLE_FUNCS in crates/ruff/src/rules/flake8_bugbear/rules/function_call_argument_default.rs.

@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Mar 27, 2023
@JonathanPlasse
Copy link
Contributor

It could be a good first issue.

@charliermarsh charliermarsh added the good first issue Good for newcomers label Mar 27, 2023
@rouge8
Copy link
Contributor Author

rouge8 commented Mar 27, 2023

Yeah perhaps we add that to the list of IMMUTABLE_FUNCS in crates/ruff/src/rules/flake8_bugbear/rules/function_call_argument_default.rs.

I think it's probably worth doing that and adding an allowlist. Looking at our codebase, we have some custom functions we'd want to allow while still benefiting from flagging this in general.

rouge8 added a commit to rouge8/ruff that referenced this issue Mar 27, 2023
rouge8 added a commit to rouge8/ruff that referenced this issue Mar 27, 2023
…`datetime.timedelta()` in a function argument defaults for B008

ref: astral-sh#3762
@rouge8
Copy link
Contributor Author

rouge8 commented Mar 27, 2023

Okay, I opened #3764 adding a few more immutable functions. I can work on another PR adding a setting if you're open to that!

@rouge8
Copy link
Contributor Author

rouge8 commented Mar 28, 2023

Oh wait, reading the code, it looks like B008 might already support extend-immutable-calls?

@charliermarsh
Copy link
Member

Oh wait, you're right...

@rouge8
Copy link
Contributor Author

rouge8 commented Mar 28, 2023

Hmm and I don't see it used in https://github.com/charliermarsh/ruff/blob/main/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs ? Is the doc saying it's used for B006 wrong and it's actually used for B008?

@charliermarsh
Copy link
Member

Yeah something's a bit off between these.

@aberres
Copy link

aberres commented Mar 29, 2023

@rouge8 pathlib.Path is another candidate. See https://peps.python.org/pep-0428/#immutability

rouge8 added a commit to rouge8/ruff that referenced this issue Mar 29, 2023
@rouge8
Copy link
Contributor Author

rouge8 commented Mar 29, 2023

@rouge8 pathlib.Path is another candidate. See https://peps.python.org/pep-0428/#immutability

Good catch! Added it in #3794

@JonathanPlasse
Copy link
Contributor

Why not (Pure)(Posix|Windows)Path too?

@charliermarsh charliermarsh added the accepted Ready for implementation label Jul 10, 2023
@Apakottur
Copy link

So at this point should it be possible to use extend-immutable-calls for B008?
I have the following code:

# test.py
from typing import TypeAlias

MyTypeAlias: TypeAlias = str

def func(
    x: str = str("default"),
    y: str = MyTypeAlias("default"),
):
    return x + y
# ruff.toml
[flake8-bugbear]
extend-immutable-calls = ["MyTypeAlias"]

And running ruff check --config ruff.toml --select B008 test.py still gives me:

test.py:9:14: B008 Do not perform function call `MyTypeAlias` in argument defaults
Found 1 error.

@charliermarsh
Copy link
Member

It should work for B008 if you use a fully-qualified path to the symbol, rather than the string name. For example:

[flake8-bugbear]
extend-immutable-calls = ["path.to.module.MyTypeAlias"]

Unfortunately, that won't work for symbols defined in the file in which they're used -- they have to be imported from another file. It's just a limitation which we'll resolve eventually (#5486).

@charliermarsh
Copy link
Member

Unrelatedly, looking back at this issue, I see a few touch-ups we can do:

  • B008 could ignore arguments with immutable annotations.
  • B006 could consider extend_immutable_calls when determining whether an annotation is immutable.

@zanieb
Copy link
Member

zanieb commented Aug 23, 2023

B008 already supports this so I'm going to close this issue.

I've also added the improvements that Charlie suggested in #6781 and #6784

@zanieb zanieb closed this as completed Aug 23, 2023
zanieb added a commit that referenced this issue Aug 24, 2023
…ts with immutable annotations (#6784)

Extends #6781 
Part of #3762
<!--
Thank you for contributing to Ruff! To help us out with reviewing,
please consider the following:

- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title?
- Does this pull request include references to any relevant issues?
-->

## Summary

<!-- What's the purpose of the change? What does it do, and why? -->
Allows calls in argument defaults if the argument is annotated as an
immutable type to avoid false positives.

## Test Plan

<!-- How was it tested? -->

Snapshots
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Ready for implementation good first issue Good for newcomers rule Implementing or modifying a lint rule
Projects
None yet
Development

No branches or pull requests

6 participants