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

Flag mutable defaults based on mutable annotations #11122

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

charliermarsh
Copy link
Member

Summary

We'll now flag def foo(a: list = LIST): ... as a mutable default based on the annotation. (If it is immutable, the annotation should be Sequence.)

I want to see what the ecosystem checks look like here. It might be too noisy.

Closes #11030.

@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Apr 24, 2024
Copy link

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+34 -0 violations, +0 -0 fixes in 4 projects; 40 projects unchanged)

commaai/openpilot (+2 -0 violations, +0 -0 fixes)

+ tools/lib/live_logreader.py:10:46: B006 Do not use mutable data structures for argument defaults
+ tools/lib/live_logreader.py:27:42: B006 Do not use mutable data structures for argument defaults

pandas-dev/pandas (+1 -0 violations, +0 -0 fixes)

+ pandas/io/formats/css.py:351:76: B006 Do not use mutable data structures for argument defaults

reflex-dev/reflex (+3 -0 violations, +0 -0 fixes)

+ reflex/app.py:418:38: B006 Do not use mutable data structures for argument defaults
+ reflex/app.py:571:38: B006 Do not use mutable data structures for argument defaults
+ reflex/utils/prerequisites.py:361:33: B006 Do not use mutable data structures for argument defaults

tiangolo/fastapi (+28 -0 violations, +0 -0 fixes)

+ docs_src/dependencies/tutorial001.py:15:38: B006 Do not use mutable data structures for argument defaults
+ docs_src/dependencies/tutorial001.py:20:38: B006 Do not use mutable data structures for argument defaults
+ docs_src/dependencies/tutorial001_py310.py:11:38: B006 Do not use mutable data structures for argument defaults
+ docs_src/dependencies/tutorial001_py310.py:16:38: B006 Do not use mutable data structures for argument defaults
+ docs_src/dependency_testing/tutorial001.py:16:38: B006 Do not use mutable data structures for argument defaults
+ docs_src/dependency_testing/tutorial001.py:21:38: B006 Do not use mutable data structures for argument defaults
+ docs_src/dependency_testing/tutorial001_py310.py:12:38: B006 Do not use mutable data structures for argument defaults
+ docs_src/dependency_testing/tutorial001_py310.py:17:38: B006 Do not use mutable data structures for argument defaults
+ docs_src/query_params_str_validations/tutorial012_py39.py:7:37: B006 Do not use mutable data structures for argument defaults
+ docs_src/query_params_str_validations/tutorial013.py:7:32: B006 Do not use mutable data structures for argument defaults
+ docs_src/request_files/tutorial002_py39.py:8:45: B006 Do not use mutable data structures for argument defaults
+ docs_src/request_files/tutorial003_py39.py:16:31: B006 Do not use mutable data structures for argument defaults
+ docs_src/request_files/tutorial003_py39.py:9:26: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_contextmanager.py:125:39: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_contextmanager.py:130:45: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_contextmanager.py:137:66: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_contextmanager.py:183:38: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_contextmanager.py:188:44: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_contextmanager.py:196:43: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_contextmanager.py:74:35: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_contextmanager.py:82:35: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_normal_exceptions.py:30:50: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_normal_exceptions.py:37:59: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_overrides.py:18:40: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_overrides.py:28:42: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_overrides.py:50:53: B006 Do not use mutable data structures for argument defaults
+ tests/test_forms_from_non_typing_sequences.py:13:38: B006 Do not use mutable data structures for argument defaults
+ tests/test_forms_from_non_typing_sequences.py:8:40: B006 Do not use mutable data structures for argument defaults

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
B006 34 34 0 0 0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+34 -0 violations, +0 -0 fixes in 4 projects; 40 projects unchanged)

commaai/openpilot (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ tools/lib/live_logreader.py:10:46: B006 Do not use mutable data structures for argument defaults
+ tools/lib/live_logreader.py:27:42: B006 Do not use mutable data structures for argument defaults

pandas-dev/pandas (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ pandas/io/formats/css.py:351:76: B006 Do not use mutable data structures for argument defaults

reflex-dev/reflex (+3 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ reflex/app.py:418:38: B006 Do not use mutable data structures for argument defaults
+ reflex/app.py:571:38: B006 Do not use mutable data structures for argument defaults
+ reflex/utils/prerequisites.py:361:33: B006 Do not use mutable data structures for argument defaults

tiangolo/fastapi (+28 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ docs_src/dependencies/tutorial001.py:15:38: B006 Do not use mutable data structures for argument defaults
+ docs_src/dependencies/tutorial001.py:20:38: B006 Do not use mutable data structures for argument defaults
+ docs_src/dependencies/tutorial001_py310.py:11:38: B006 Do not use mutable data structures for argument defaults
+ docs_src/dependencies/tutorial001_py310.py:16:38: B006 Do not use mutable data structures for argument defaults
+ docs_src/dependency_testing/tutorial001.py:16:38: B006 Do not use mutable data structures for argument defaults
+ docs_src/dependency_testing/tutorial001.py:21:38: B006 Do not use mutable data structures for argument defaults
+ docs_src/dependency_testing/tutorial001_py310.py:12:38: B006 Do not use mutable data structures for argument defaults
+ docs_src/dependency_testing/tutorial001_py310.py:17:38: B006 Do not use mutable data structures for argument defaults
+ docs_src/query_params_str_validations/tutorial012_py39.py:7:37: B006 Do not use mutable data structures for argument defaults
+ docs_src/query_params_str_validations/tutorial013.py:7:32: B006 Do not use mutable data structures for argument defaults
+ docs_src/request_files/tutorial002_py39.py:8:45: B006 Do not use mutable data structures for argument defaults
+ docs_src/request_files/tutorial003_py39.py:16:31: B006 Do not use mutable data structures for argument defaults
+ docs_src/request_files/tutorial003_py39.py:9:26: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_contextmanager.py:125:39: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_contextmanager.py:130:45: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_contextmanager.py:137:66: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_contextmanager.py:183:38: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_contextmanager.py:188:44: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_contextmanager.py:196:43: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_contextmanager.py:74:35: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_contextmanager.py:82:35: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_normal_exceptions.py:30:50: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_normal_exceptions.py:37:59: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_overrides.py:18:40: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_overrides.py:28:42: B006 Do not use mutable data structures for argument defaults
+ tests/test_dependency_overrides.py:50:53: B006 Do not use mutable data structures for argument defaults
+ tests/test_forms_from_non_typing_sequences.py:13:38: B006 Do not use mutable data structures for argument defaults
+ tests/test_forms_from_non_typing_sequences.py:8:40: B006 Do not use mutable data structures for argument defaults

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
B006 34 34 0 0 0

@charliermarsh charliermarsh marked this pull request as draft April 24, 2024 03:35
@charliermarsh
Copy link
Member Author

Just gonna mark as draft since I haven’t had a chance to review the ecosystem results and I’m going to bed.

@dhruvmanila
Copy link
Member

I haven't looked at the code nor the ecosystem checks, but I think my suggestion was to use the TypeChecker to determine if the variable is assigned to a mutable type

/// Abstraction for a type checker, conservatively checks for the intended type(s).
pub trait TypeChecker {
/// Check annotation expression to match the intended type(s).
fn match_annotation(annotation: &Expr, semantic: &SemanticModel) -> bool;
/// Check initializer expression to match the intended type(s).
fn match_initializer(initializer: &Expr, semantic: &SemanticModel) -> bool;
}

But, I think this could be fine as well.

checker.semantic(),
extend_immutable_calls.as_slice(),
)
}))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(At the very least I think these should return enums rather than bools, with a single call to check immutability.)

@zanieb
Copy link
Member

zanieb commented Apr 24, 2024

The ecosystem checks look good to me, except the FastAPI ones which are... hard to comment on. I'm not sure if they can use this rule?

I'd gate this with preview.

@charliermarsh
Copy link
Member Author

(I think we generally recommend adding Query to extend-immutable-types or whatever the setting is called.)

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

Successfully merging this pull request may close these issues.

Mutable default argument not detected when it is a constant (by B006 and others)
3 participants