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

Implement pylint's else-if-used rule (PLR5501) #3231

Merged
merged 5 commits into from
Feb 26, 2023
Merged
Show file tree
Hide file tree
Changes from 2 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
47 changes: 47 additions & 0 deletions crates/ruff/resources/test/fixtures/pylint/else_if_used.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
"""
Test for else-if-used
"""
def ok0():
"""Should not trigger on elif"""
if 1:
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is being flagged as an error in the fixture, when it should be considered ok.

pass
elif 2:
pass


def ok1():
"""If the orelse has more than 1 item in it, shouldn't trigger"""
if 1:
pass
else:
print()
if 1:
pass


def ok2():
"""If the orelse has more than 1 item in it, shouldn't trigger"""
if 1:
pass
else:
if 1:
pass
print()


def not_ok0():
if 1:
pass
else:
if 2:
pass


def not_ok1():
if 1:
pass
else:
if 2:
pass
else:
pass
7 changes: 7 additions & 0 deletions crates/ruff/src/checkers/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1604,6 +1604,13 @@ where
if self.settings.rules.enabled(&Rule::OutdatedVersionBlock) {
pyupgrade::rules::outdated_version_block(self, stmt, test, body, orelse);
}
if self.settings.rules.enabled(&Rule::ElseIfUsed) {
if let Some(diagnostic) =
pylint::rules::else_if_used(stmt, body, orelse, self.locator)
{
self.diagnostics.push(diagnostic);
}
}
}
StmtKind::Assert { test, msg } => {
if self.settings.rules.enabled(&Rule::AssertTuple) {
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 @@ -143,6 +143,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
(Pylint, "R0133") => Rule::ComparisonOfConstant,
(Pylint, "R1701") => Rule::ConsiderMergingIsinstance,
(Pylint, "R1722") => Rule::ConsiderUsingSysExit,
(Pylint, "R5501") => Rule::ElseIfUsed,
(Pylint, "R2004") => Rule::MagicValueComparison,
(Pylint, "W0120") => Rule::UselessElseOnLoop,
(Pylint, "W0602") => Rule::GlobalVariableNotAssigned,
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 @@ -133,6 +133,7 @@ ruff_macros::register_rules!(
rules::pylint::rules::BadStringFormatType,
rules::pylint::rules::BidirectionalUnicode,
rules::pylint::rules::BadStrStripCall,
rules::pylint::rules::ElseIfUsed,
rules::pylint::rules::UselessImportAlias,
rules::pylint::rules::UnnecessaryDirectLambdaCall,
rules::pylint::rules::NonlocalWithoutBinding,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ mod tests {
#[test_case(Rule::TooManyArguments, Path::new("too_many_arguments.py"); "PLR0913")]
#[test_case(Rule::TooManyBranches, Path::new("too_many_branches.py"); "PLR0912")]
#[test_case(Rule::TooManyStatements, Path::new("too_many_statements.py"); "PLR0915")]
#[test_case(Rule::ElseIfUsed, Path::new("else_if_used.py"); "PLR5501")]
#[test_case(Rule::BadStringFormatType, Path::new("bad_string_format_type.py"); "PLE1307")]
#[test_case(Rule::BidirectionalUnicode, Path::new("bidirectional_unicode.py"); "PLE2502")]
#[test_case(Rule::BadStrStripCall, Path::new("bad_str_strip_call.py"); "PLE01310")]
Expand Down
45 changes: 45 additions & 0 deletions crates/ruff/src/rules/pylint/rules/else_if_used.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
use ruff_macros::{define_violation, derive_message_formats};
use rustpython_parser::ast::{Stmt, StmtKind};

use crate::ast::helpers::identifier_range;
use crate::registry::Diagnostic;
use crate::source_code::Locator;
use crate::violation::Violation;

define_violation!(
pub struct ElseIfUsed {}
);

impl Violation for ElseIfUsed {
#[derive_message_formats]
fn message(&self) -> String {
format!("Consider using \"elif\" instead of \"else\" then \"if\" to remove one indentation level")
}
}

/// PLR5501
pub fn else_if_used(
stmt: &Stmt,
body: &[Stmt],
_orelse: &[Stmt],
locator: &Locator,
) -> Option<Diagnostic> {
if let StmtKind::If { orelse, .. } = &stmt.node {
Copy link
Member

Choose a reason for hiding this comment

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

Can we not use the orelse passed into the function already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you're right, going to give that a go.

// If the body contains more than just the orelse, can't apply.
if body.len() == 1 {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be orelse.len() == 1? I thought we'd be looking for orelse.len() == 1 && matches!(orelse[0].node, StmtKind::If { .. }), which typically indicates an elif (or an else: if: in this case, which we'd have to differentiate somehow).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, here stmt is the if in the else: if: .... I'd assumed stmt was the outer if in:

if ...:
  pass
else:
  if:
    ...

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think we do need to structure this such that it looks at this statement:

if ...:
  pass
else:
  if:
    ...

Rather than the nested if. Otherwise, how can you tell if you're in an else or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is more complicated than I thought, namely because you don't know where the else is.
Pylint does have separate elif tokens which makes it easier...the only way I can think of doing it might be a bit janky where you use the locator to see if 'elif' is in the line or something like that.

Copy link
Contributor Author

@chanman3388 chanman3388 Feb 26, 2023

Choose a reason for hiding this comment

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

So this can be done via the text method...is that ok? It feels a bit...dirty...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it's ok. The alternative would be to do it by lexing and looking at the token stream. But this is effectively equivalent. There's no way to detect this AFAIK from the AST alone.

if let [first, ..] = &orelse[..] {
if matches!(first.node, StmtKind::If { .. }) {
// check the source if this is else then if or elif
// we do this to see if they are on the same line
if stmt.location.row() != first.location.row() {
return Some(Diagnostic::new(
ElseIfUsed {},
identifier_range(stmt, locator),
));
}
}
}
}
}
None
}
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ pub use bad_string_format_type::{bad_string_format_type, BadStringFormatType};
pub use bidirectional_unicode::{bidirectional_unicode, BidirectionalUnicode};
pub use comparison_of_constant::{comparison_of_constant, ComparisonOfConstant};
pub use consider_using_sys_exit::{consider_using_sys_exit, ConsiderUsingSysExit};
pub use else_if_used::{else_if_used, ElseIfUsed};
pub use global_variable_not_assigned::GlobalVariableNotAssigned;
pub use invalid_all_format::{invalid_all_format, InvalidAllFormat};
pub use invalid_all_object::{invalid_all_object, InvalidAllObject};
Expand Down Expand Up @@ -35,6 +36,7 @@ mod bad_string_format_type;
mod bidirectional_unicode;
mod comparison_of_constant;
mod consider_using_sys_exit;
mod else_if_used;
mod global_variable_not_assigned;
mod invalid_all_format;
mod invalid_all_object;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
---
source: crates/ruff/src/rules/pylint/mod.rs
expression: diagnostics
---
- kind:
ElseIfUsed: {}
location:
row: 6
column: 4
end_location:
row: 9
column: 12
fix: ~
parent: ~
- kind:
ElseIfUsed: {}
location:
row: 24
column: 4
end_location:
row: 29
column: 15
fix: ~
parent: ~
- kind:
ElseIfUsed: {}
location:
row: 33
column: 4
end_location:
row: 37
column: 16
fix: ~
parent: ~
- kind:
ElseIfUsed: {}
location:
row: 41
column: 4
end_location:
row: 47
column: 16
fix: ~
parent: ~

4 changes: 4 additions & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.