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

[flake8-boolean-trap] Add setting for user defined allowed boolean trap #10531

Merged
merged 5 commits into from
Mar 30, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -132,3 +132,26 @@ def __post_init__(self, force: bool) -> None:
print(force)

Fit(force=True)


# https://github.com/astral-sh/ruff/issues/10356
from django.db.models import Case, Q, Value, When


qs.annotate(
is_foo_or_bar=Case(
When(Q(is_foo=True) | Q(is_bar=True)),
then=Value(True),
),
default=Value(False),
)


# https://github.com/astral-sh/ruff/issues/10485
from pydantic import Field
from pydantic_settings import BaseSettings


class Settings(BaseSettings):

foo: bool = Field(True, exclude=True)
17 changes: 16 additions & 1 deletion crates/ruff_linter/src/rules/flake8_boolean_trap/helpers.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::settings::LinterSettings;
use ruff_python_ast::{self as ast, Expr};

/// Returns `true` if a function call is allowed to use a boolean trap.
Expand Down Expand Up @@ -43,6 +44,14 @@ pub(super) fn is_allowed_func_call(name: &str) -> bool {
)
}

/// Returns `true` if a function call is allowed by the user to use a boolean trap.
pub(super) fn is_user_allowed_func_call(name: &str, settings: &LinterSettings) -> bool {
settings
.flake8_boolean_trap
.extend_allowed_calls
.contains(&name.to_string())
}

/// Returns `true` if a function definition is allowed to use a boolean trap.
pub(super) fn is_allowed_func_def(name: &str) -> bool {
matches!(name, "__setitem__" | "__post_init__")
Expand All @@ -51,7 +60,7 @@ pub(super) fn is_allowed_func_def(name: &str) -> bool {
/// Returns `true` if an argument is allowed to use a boolean trap. To return
/// `true`, the function name must be explicitly allowed, and the argument must
/// be either the first or second argument in the call.
pub(super) fn allow_boolean_trap(call: &ast::ExprCall) -> bool {
pub(super) fn allow_boolean_trap(call: &ast::ExprCall, settings: &LinterSettings) -> bool {
let func_name = match call.func.as_ref() {
Expr::Attribute(ast::ExprAttribute { attr, .. }) => attr.as_str(),
Expr::Name(ast::ExprName { id, .. }) => id.as_str(),
Expand All @@ -76,5 +85,11 @@ pub(super) fn allow_boolean_trap(call: &ast::ExprCall) -> bool {
}
}

// If the function name is explicitly allowed by the user, then the boolean trap is
// allowed.
if is_user_allowed_func_call(func_name, settings) {
return true;
}

false
}
23 changes: 23 additions & 0 deletions crates/ruff_linter/src/rules/flake8_boolean_trap/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! Rules from [flake8-boolean-trap](https://pypi.org/project/flake8-boolean-trap/).
mod helpers;
pub(crate) mod rules;
pub mod settings;

#[cfg(test)]
mod tests {
Expand All @@ -11,6 +12,7 @@ mod tests {

use crate::registry::Rule;
use crate::settings::types::PreviewMode;
use crate::settings::LinterSettings;
use crate::test::test_path;
use crate::{assert_messages, settings};

Expand Down Expand Up @@ -44,4 +46,25 @@ mod tests {
assert_messages!(snapshot, diagnostics);
Ok(())
}

#[test]
fn extend_allowed_callable() -> Result<()> {
let diagnostics = test_path(
Path::new("flake8_boolean_trap/FBT.py"),
&LinterSettings {
flake8_boolean_trap: super::settings::Settings {
extend_allowed_calls: vec![
"Value".to_string(),
"Field".to_string(),
"settings".to_string(),
"deploy".to_string(),
"used".to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me if these should be function names, or symbol paths (like ["pydantic", "BaseModel"]).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean it's not clear in the documentation/implementation? Or are you saying you're not sure how we should implement it?

Copy link
Member

Choose a reason for hiding this comment

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

The latter!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think symbol path gives finer control, and is less prone to false negatives. The only downside is that it is less intuitive to users, and may lead to confusion. But I think this should be ok with good documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change it to symbol paths if everyone agrees.

Copy link

Choose a reason for hiding this comment

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

is it hard to support both?

Copy link
Member

Choose a reason for hiding this comment

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

Symbol path is a little problematic because it doesn't support methods (e.g., there's no way to identify y in x = pydantic.BaseModel(); x.y()), and methods are a common use-case here.

],
},
..LinterSettings::for_rule(Rule::BooleanPositionalValueInCall)
},
)?;
assert_messages!(diagnostics);
Ok(())
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ use crate::rules::flake8_boolean_trap::helpers::allow_boolean_trap;
/// ## What it does
/// Checks for boolean positional arguments in function calls.
///
/// Some functions are whitelisted by default. To extend the list of allowed calls
/// configure the [`lint.flake8-boolean-trap.extend-allowed-calls`] option.
///
/// ## Why is this bad?
/// Calling a function with boolean positional arguments is confusing as the
/// meaning of the boolean value is not clear to the caller, and to future
Expand All @@ -32,6 +35,9 @@ use crate::rules::flake8_boolean_trap::helpers::allow_boolean_trap;
/// func(flag=True)
/// ```
///
/// ## Options
/// - `lint.flake8-boolean-trap.extend-allowed-calls`
///
/// ## References
/// - [Python documentation: Calls](https://docs.python.org/3/reference/expressions.html#calls)
/// - [_How to Avoid “The Boolean Trap”_ by Adam Johnson](https://adamj.eu/tech/2021/07/10/python-type-hints-how-to-avoid-the-boolean-trap/)
Expand All @@ -46,7 +52,7 @@ impl Violation for BooleanPositionalValueInCall {
}

pub(crate) fn boolean_positional_value_in_call(checker: &mut Checker, call: &ast::ExprCall) {
if allow_boolean_trap(call) {
if allow_boolean_trap(call, checker.settings) {
return;
}
for arg in call
Expand Down
23 changes: 23 additions & 0 deletions crates/ruff_linter/src/rules/flake8_boolean_trap/settings.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
//! Settings for the `flake8_boolean_trap` plugin.

use crate::display_settings;
use ruff_macros::CacheKey;
use std::fmt;

#[derive(Debug, CacheKey, Default)]
pub struct Settings {
pub extend_allowed_calls: Vec<String>,
}

impl fmt::Display for Settings {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
display_settings! {
formatter = f,
namespace = "linter.flake8_boolean_trap",
fields = [
self.extend_allowed_calls | array,
]
}
Ok(())
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,29 @@ FBT.py:121:10: FBT003 Boolean positional value in function call
| ^^^^ FBT003
|

FBT.py:144:20: FBT003 Boolean positional value in function call
|
142 | is_foo_or_bar=Case(
143 | When(Q(is_foo=True) | Q(is_bar=True)),
144 | then=Value(True),
| ^^^^ FBT003
145 | ),
146 | default=Value(False),
|

FBT.py:146:19: FBT003 Boolean positional value in function call
|
144 | then=Value(True),
145 | ),
146 | default=Value(False),
| ^^^^^ FBT003
147 | )
|

FBT.py:157:23: FBT003 Boolean positional value in function call
|
155 | class Settings(BaseSettings):
156 |
157 | foo: bool = Field(True, exclude=True)
| ^^^^ FBT003
|
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
source: crates/ruff_linter/src/rules/flake8_boolean_trap/mod.rs
---

14 changes: 8 additions & 6 deletions crates/ruff_linter/src/settings/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@ use ruff_macros::CacheKey;
use crate::line_width::LineLength;
use crate::registry::{Linter, Rule};
use crate::rules::{
flake8_annotations, flake8_bandit, flake8_bugbear, flake8_builtins, flake8_comprehensions,
flake8_copyright, flake8_errmsg, flake8_gettext, flake8_implicit_str_concat,
flake8_import_conventions, flake8_pytest_style, flake8_quotes, flake8_self,
flake8_tidy_imports, flake8_type_checking, flake8_unused_arguments, isort, mccabe, pep8_naming,
pycodestyle, pydocstyle, pyflakes, pylint, pyupgrade,
flake8_annotations, flake8_bandit, flake8_boolean_trap, flake8_bugbear, flake8_builtins,
flake8_comprehensions, flake8_copyright, flake8_errmsg, flake8_gettext,
flake8_implicit_str_concat, flake8_import_conventions, flake8_pytest_style, flake8_quotes,
flake8_self, flake8_tidy_imports, flake8_type_checking, flake8_unused_arguments, isort, mccabe,
pep8_naming, pycodestyle, pydocstyle, pyflakes, pylint, pyupgrade,
};
use crate::settings::types::{ExtensionMapping, FilePatternSet, PerFileIgnores, PythonVersion};
use crate::{codes, RuleSelector};
Expand Down Expand Up @@ -237,6 +237,7 @@ pub struct LinterSettings {
// Plugins
pub flake8_annotations: flake8_annotations::settings::Settings,
pub flake8_bandit: flake8_bandit::settings::Settings,
pub flake8_boolean_trap: flake8_boolean_trap::settings::Settings,
pub flake8_bugbear: flake8_bugbear::settings::Settings,
pub flake8_builtins: flake8_builtins::settings::Settings,
pub flake8_comprehensions: flake8_comprehensions::settings::Settings,
Expand Down Expand Up @@ -399,16 +400,17 @@ impl LinterSettings {
typing_modules: vec![],
flake8_annotations: flake8_annotations::settings::Settings::default(),
flake8_bandit: flake8_bandit::settings::Settings::default(),
flake8_boolean_trap: flake8_boolean_trap::settings::Settings::default(),
flake8_bugbear: flake8_bugbear::settings::Settings::default(),
flake8_builtins: flake8_builtins::settings::Settings::default(),
flake8_comprehensions: flake8_comprehensions::settings::Settings::default(),
flake8_copyright: flake8_copyright::settings::Settings::default(),
flake8_errmsg: flake8_errmsg::settings::Settings::default(),
flake8_gettext: flake8_gettext::settings::Settings::default(),
flake8_implicit_str_concat: flake8_implicit_str_concat::settings::Settings::default(),
flake8_import_conventions: flake8_import_conventions::settings::Settings::default(),
flake8_pytest_style: flake8_pytest_style::settings::Settings::default(),
flake8_quotes: flake8_quotes::settings::Settings::default(),
flake8_gettext: flake8_gettext::settings::Settings::default(),
flake8_self: flake8_self::settings::Settings::default(),
flake8_tidy_imports: flake8_tidy_imports::settings::Settings::default(),
flake8_type_checking: flake8_type_checking::settings::Settings::default(),
Expand Down
20 changes: 16 additions & 4 deletions crates/ruff_workspace/src/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,11 @@ use ruff_python_formatter::{
};

use crate::options::{
Flake8AnnotationsOptions, Flake8BanditOptions, Flake8BugbearOptions, Flake8BuiltinsOptions,
Flake8ComprehensionsOptions, Flake8CopyrightOptions, Flake8ErrMsgOptions, Flake8GetTextOptions,
Flake8ImplicitStrConcatOptions, Flake8ImportConventionsOptions, Flake8PytestStyleOptions,
Flake8QuotesOptions, Flake8SelfOptions, Flake8TidyImportsOptions, Flake8TypeCheckingOptions,
Flake8AnnotationsOptions, Flake8BanditOptions, Flake8BooleanTrapOptions, Flake8BugbearOptions,
Flake8BuiltinsOptions, Flake8ComprehensionsOptions, Flake8CopyrightOptions,
Flake8ErrMsgOptions, Flake8GetTextOptions, Flake8ImplicitStrConcatOptions,
Flake8ImportConventionsOptions, Flake8PytestStyleOptions, Flake8QuotesOptions,
Flake8SelfOptions, Flake8TidyImportsOptions, Flake8TypeCheckingOptions,
Flake8UnusedArgumentsOptions, FormatOptions, IsortOptions, LintCommonOptions, LintOptions,
McCabeOptions, Options, Pep8NamingOptions, PyUpgradeOptions, PycodestyleOptions,
PydocstyleOptions, PyflakesOptions, PylintOptions,
Expand Down Expand Up @@ -292,6 +293,10 @@ impl Configuration {
.flake8_bandit
.map(Flake8BanditOptions::into_settings)
.unwrap_or_default(),
flake8_boolean_trap: lint
.flake8_boolean_trap
.map(Flake8BooleanTrapOptions::into_settings)
.unwrap_or_default(),
flake8_bugbear: lint
.flake8_bugbear
.map(Flake8BugbearOptions::into_settings)
Expand Down Expand Up @@ -609,6 +614,7 @@ pub struct LintConfiguration {
// Plugins
pub flake8_annotations: Option<Flake8AnnotationsOptions>,
pub flake8_bandit: Option<Flake8BanditOptions>,
pub flake8_boolean_trap: Option<Flake8BooleanTrapOptions>,
pub flake8_bugbear: Option<Flake8BugbearOptions>,
pub flake8_builtins: Option<Flake8BuiltinsOptions>,
pub flake8_comprehensions: Option<Flake8ComprehensionsOptions>,
Expand Down Expand Up @@ -713,6 +719,7 @@ impl LintConfiguration {
// Plugins
flake8_annotations: options.common.flake8_annotations,
flake8_bandit: options.common.flake8_bandit,
flake8_boolean_trap: options.common.flake8_boolean_trap,
flake8_bugbear: options.common.flake8_bugbear,
flake8_builtins: options.common.flake8_builtins,
flake8_comprehensions: options.common.flake8_comprehensions,
Expand Down Expand Up @@ -1127,6 +1134,7 @@ impl LintConfiguration {
// Plugins
flake8_annotations: self.flake8_annotations.combine(config.flake8_annotations),
flake8_bandit: self.flake8_bandit.combine(config.flake8_bandit),
flake8_boolean_trap: self.flake8_boolean_trap.combine(config.flake8_boolean_trap),
flake8_bugbear: self.flake8_bugbear.combine(config.flake8_bugbear),
flake8_builtins: self.flake8_builtins.combine(config.flake8_builtins),
flake8_comprehensions: self
Expand Down Expand Up @@ -1358,6 +1366,10 @@ fn warn_about_deprecated_top_level_lint_options(
used_options.push("flake8-bandit");
}

if top_level_options.flake8_boolean_trap.is_some() {
used_options.push("flake8-boolean-trap");
}

if top_level_options.flake8_bugbear.is_some() {
used_options.push("flake8-bugbear");
}
Expand Down
27 changes: 27 additions & 0 deletions crates/ruff_workspace/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -809,6 +809,10 @@ pub struct LintCommonOptions {
#[option_group]
pub flake8_bandit: Option<Flake8BanditOptions>,

/// Options for the `flake8-boolean-trap` plugin.
#[option_group]
pub flake8_boolean_trap: Option<Flake8BooleanTrapOptions>,

/// Options for the `flake8-bugbear` plugin.
#[option_group]
pub flake8_bugbear: Option<Flake8BugbearOptions>,
Expand Down Expand Up @@ -1046,6 +1050,29 @@ impl Flake8BanditOptions {
}
}

#[derive(
Clone, Debug, PartialEq, Eq, Default, Serialize, Deserialize, OptionsMetadata, CombineOptions,
)]
#[serde(deny_unknown_fields, rename_all = "kebab-case")]
#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))]
pub struct Flake8BooleanTrapOptions {
/// A list of calls with which to allow a boolean trap.
#[option(
default = "[]",
value_type = "list[str]",
example = "extend-allowed-calls = [\"Field\", \"Value\"]"
)]
pub extend_allowed_calls: Option<Vec<String>>,
}

impl Flake8BooleanTrapOptions {
pub fn into_settings(self) -> ruff_linter::rules::flake8_boolean_trap::settings::Settings {
ruff_linter::rules::flake8_boolean_trap::settings::Settings {
extend_allowed_calls: self.extend_allowed_calls.unwrap_or_default(),
}
}
}

#[derive(
Clone, Debug, PartialEq, Eq, Default, Serialize, Deserialize, OptionsMetadata, CombineOptions,
)]
Expand Down