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-import-conventions] Add a rule for BannedImportAlias #3926

Merged
merged 2 commits into from
Apr 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import typing as t # banned
import typing as ty # banned

import numpy as nmp # banned
import numpy as npy # banned
import tensorflow.keras.backend as K # banned
import torch.nn.functional as F # banned
from tensorflow.keras import backend as K # banned
from torch.nn import functional as F # banned

from typing import Any # ok

import numpy as np # ok
import tensorflow as tf # ok
import torch.nn as nn # ok
from tensorflow.keras import backend # ok
35 changes: 35 additions & 0 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1077,6 +1077,21 @@ where
}
}

if self.settings.rules.enabled(Rule::BannedImportAlias) {
if let Some(asname) = &alias.node.asname {
if let Some(diagnostic) =
flake8_import_conventions::rules::check_banned_import(
stmt,
&alias.node.name,
asname,
&self.settings.flake8_import_conventions.banned_aliases,
)
{
self.diagnostics.push(diagnostic);
}
}
}

if self
.settings
.rules
Expand Down Expand Up @@ -1339,6 +1354,26 @@ where
}
}

if self.settings.rules.enabled(Rule::BannedImportAlias) {
if let Some(asname) = &alias.node.asname {
let full_name = helpers::format_import_from_member(
*level,
module.as_deref(),
&alias.node.name,
);
if let Some(diagnostic) =
flake8_import_conventions::rules::check_banned_import(
stmt,
&full_name,
asname,
&self.settings.flake8_import_conventions.banned_aliases,
)
{
self.diagnostics.push(diagnostic);
}
}
}

if let Some(asname) = &alias.node.asname {
if self
.settings
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {

// flake8-import-conventions
(Flake8ImportConventions, "001") => Rule::UnconventionalImportAlias,
(Flake8ImportConventions, "002") => Rule::BannedImportAlias,

// flake8-datetimez
(Flake8Datetimez, "001") => Rule::CallDatetimeWithoutTzinfo,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,7 @@ ruff_macros::register_rules!(
rules::flake8_unused_arguments::rules::UnusedLambdaArgument,
// flake8-import-conventions
rules::flake8_import_conventions::rules::UnconventionalImportAlias,
rules::flake8_import_conventions::rules::BannedImportAlias,
// flake8-datetimez
rules::flake8_datetimez::rules::CallDatetimeWithoutTzinfo,
rules::flake8_datetimez::rules::CallDatetimeToday,
Expand Down
36 changes: 36 additions & 0 deletions crates/ruff/src/rules/flake8_import_conventions/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ mod tests {
("dask.array".to_string(), "da".to_string()),
("dask.dataframe".to_string(), "dd".to_string()),
])),
banned_aliases: None,
}
.into(),
..Settings::for_rule(Rule::UnconventionalImportAlias)
Expand All @@ -45,6 +46,38 @@ mod tests {
Ok(())
}

#[test]
fn custom_banned() -> Result<()> {
let diagnostics = test_path(
Path::new("flake8_import_conventions/custom_banned.py"),
&Settings {
flake8_import_conventions: super::settings::Options {
aliases: None,
extend_aliases: None,
banned_aliases: Some(FxHashMap::from_iter([
(
"typing".to_string(),
vec!["t".to_string(), "ty".to_string()],
),
(
"numpy".to_string(),
vec!["nmp".to_string(), "npy".to_string()],
),
(
"tensorflow.keras.backend".to_string(),
vec!["K".to_string()],
),
("torch.nn.functional".to_string(), vec!["F".to_string()]),
])),
}
.into(),
..Settings::for_rule(Rule::BannedImportAlias)
},
)?;
assert_messages!("custom_banned", diagnostics);
Ok(())
}

#[test]
fn remove_defaults() -> Result<()> {
let diagnostics = test_path(
Expand All @@ -58,6 +91,7 @@ mod tests {
("seaborn".to_string(), "sns".to_string()),
])),
extend_aliases: None,
banned_aliases: None,
}
.into(),
..Settings::for_rule(Rule::UnconventionalImportAlias)
Expand All @@ -78,6 +112,7 @@ mod tests {
"numpy".to_string(),
"nmp".to_string(),
)])),
banned_aliases: None,
}
.into(),
..Settings::for_rule(Rule::UnconventionalImportAlias)
Expand All @@ -101,6 +136,7 @@ mod tests {
"pstr".to_string(),
),
])),
banned_aliases: None,
}
.into(),
..Settings::for_rule(Rule::UnconventionalImportAlias)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
use rustc_hash::FxHashMap;
use rustpython_parser::ast::Stmt;

use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::types::Range;

/// ## What it does
/// Checks for imports that use non-standard naming conventions, like
/// `import tensorflow.keras.backend as K`.
///
/// ## Why is this bad?
/// Consistency is good. Avoid using a non-standard naming convention for
/// imports, and, in particular, choosing import aliases that violate PEP 8.
///
/// For example, aliasing via `import tensorflow.keras.backend as K` violates
/// the guidance of PEP 8, and is thus avoided in some projects.
///
/// ## Example
/// ```python
/// import tensorflow.keras.backend as K
/// ```
///
/// Use instead:
/// ```python
/// import tensorflow as tf
///
/// tf.keras.backend
/// ```
#[violation]
pub struct BannedImportAlias(pub String, pub String);

impl Violation for BannedImportAlias {
#[derive_message_formats]
fn message(&self) -> String {
let BannedImportAlias(name, asname) = self;
format!("`{name}` should not be imported as `{asname}`")
}
}

/// ICN002
pub fn check_banned_import(
import_from: &Stmt,
name: &str,
asname: &str,
banned_conventions: &FxHashMap<String, Vec<String>>,
) -> Option<Diagnostic> {
if let Some(banned_aliases) = banned_conventions.get(name) {
if banned_aliases
.iter()
.any(|banned_alias| banned_alias == asname)
Copy link
Member

Choose a reason for hiding this comment

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

I tweaked the logic here a bit, I think it was unintentionally flagging any non-aliased import.

{
return Some(Diagnostic::new(
BannedImportAlias(name.to_string(), asname.to_string()),
Range::from(import_from),
));
}
}
None
}
5 changes: 5 additions & 0 deletions crates/ruff/src/rules/flake8_import_conventions/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pub use check_banned_import::{check_banned_import, BannedImportAlias};
pub use check_conventional_import::{check_conventional_import, UnconventionalImportAlias};

mod check_banned_import;
mod check_conventional_import;
32 changes: 27 additions & 5 deletions crates/ruff/src/rules/flake8_import_conventions/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,26 @@ pub struct Options {
"dask.dataframe" = "dd"
"#
)]
/// A mapping of modules to their conventional import aliases. These aliases
/// will be added to the `aliases` mapping.
/// A mapping from module to conventional import alias. These aliases will
/// be added to the `aliases` mapping.
pub extend_aliases: Option<FxHashMap<String, String>>,
#[option(
default = r#"{}"#,
value_type = "dict[str, list[str]]",
example = r#"
[tool.ruff.flake8-import-conventions.banned-aliases]
# Declare the banned aliases.
"tensorflow.keras.backend" = ["K"]
"#
)]
/// A mapping from module to its banned import aliases.
pub banned_aliases: Option<FxHashMap<String, Vec<String>>>,
}

#[derive(Debug, CacheKey)]
pub struct Settings {
pub aliases: FxHashMap<String, String>,
pub banned_aliases: FxHashMap<String, Vec<String>>,
}

fn default_aliases() -> FxHashMap<String, String> {
Expand All @@ -73,29 +85,38 @@ fn default_aliases() -> FxHashMap<String, String> {
.collect::<FxHashMap<_, _>>()
}

fn resolve_aliases(options: Options) -> FxHashMap<String, String> {
fn resolve_aliases(
options: Options,
) -> (FxHashMap<String, String>, FxHashMap<String, Vec<String>>) {
let mut aliases = match options.aliases {
Some(options_aliases) => options_aliases,
None => default_aliases(),
};
if let Some(extend_aliases) = options.extend_aliases {
aliases.extend(extend_aliases);
}
aliases
let banned_aliases = match options.banned_aliases {
Some(options_banned_aliases) => options_banned_aliases,
None => FxHashMap::default(),
};
(aliases, banned_aliases)
}

impl Default for Settings {
fn default() -> Self {
Self {
aliases: default_aliases(),
banned_aliases: FxHashMap::default(),
}
}
}

impl From<Options> for Settings {
fn from(options: Options) -> Self {
let (aliases, banned_aliases) = resolve_aliases(options);
Self {
aliases: resolve_aliases(options),
aliases,
banned_aliases,
}
}
}
Expand All @@ -105,6 +126,7 @@ impl From<Settings> for Options {
Self {
aliases: Some(settings.aliases),
extend_aliases: None,
banned_aliases: None,
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
---
source: crates/ruff/src/rules/flake8_import_conventions/mod.rs
---
custom_banned.py:1:1: ICN002 `typing` should not be imported as `t`
|
1 | import typing as t # banned
| ^^^^^^^^^^^^^^^^^^ ICN002
2 | import typing as ty # banned
|

custom_banned.py:2:1: ICN002 `typing` should not be imported as `ty`
|
2 | import typing as t # banned
3 | import typing as ty # banned
| ^^^^^^^^^^^^^^^^^^^ ICN002
4 |
5 | import numpy as nmp # banned
|

custom_banned.py:4:1: ICN002 `numpy` should not be imported as `nmp`
|
4 | import typing as ty # banned
5 |
6 | import numpy as nmp # banned
| ^^^^^^^^^^^^^^^^^^^ ICN002
7 | import numpy as npy # banned
8 | import tensorflow.keras.backend as K # banned
|

custom_banned.py:5:1: ICN002 `numpy` should not be imported as `npy`
|
5 | import numpy as nmp # banned
6 | import numpy as npy # banned
| ^^^^^^^^^^^^^^^^^^^ ICN002
7 | import tensorflow.keras.backend as K # banned
8 | import torch.nn.functional as F # banned
|

custom_banned.py:6:1: ICN002 `tensorflow.keras.backend` should not be imported as `K`
|
6 | import numpy as nmp # banned
7 | import numpy as npy # banned
8 | import tensorflow.keras.backend as K # banned
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ICN002
9 | import torch.nn.functional as F # banned
10 | from tensorflow.keras import backend as K # banned
|

custom_banned.py:7:1: ICN002 `torch.nn.functional` should not be imported as `F`
|
7 | import numpy as npy # banned
8 | import tensorflow.keras.backend as K # banned
9 | import torch.nn.functional as F # banned
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ICN002
10 | from tensorflow.keras import backend as K # banned
11 | from torch.nn import functional as F # banned
|

custom_banned.py:8:1: ICN002 `tensorflow.keras.backend` should not be imported as `K`
|
8 | import tensorflow.keras.backend as K # banned
9 | import torch.nn.functional as F # banned
10 | from tensorflow.keras import backend as K # banned
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ICN002
11 | from torch.nn import functional as F # banned
|

custom_banned.py:9:1: ICN002 `torch.nn.functional` should not be imported as `F`
|
9 | import torch.nn.functional as F # banned
10 | from tensorflow.keras import backend as K # banned
11 | from torch.nn import functional as F # banned
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ICN002
12 |
13 | from typing import Any # ok
|


1 change: 1 addition & 0 deletions crates/ruff/src/settings/pyproject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,7 @@ other-attribute = 1
"dask.dataframe".to_string(),
"dd".to_string(),
)])),
banned_aliases: None,
}),
mccabe: Some(mccabe::settings::Options {
max_complexity: Some(10),
Expand Down
Loading