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

[pylint] Implement invalid-length-returned (E0303) #10963

Merged

Conversation

tibor-reiss
Copy link
Contributor

@tibor-reiss tibor-reiss commented Apr 15, 2024

Add pylint rule invalid-length-returned (PLE0303)

See #970 for rules

Test Plan: cargo test

TBD: from the description: "Strictly speaking bool is a subclass of int, thus returning True/False is valid. To be consistent with other rules (e.g. PLE0305 invalid-index-returned), ruff will raise, compared to pylint which will not raise."

@tibor-reiss
Copy link
Contributor Author

The tests run locally fine when using commit 670d66f as rebase, but there are failures when using the next commit, c221035. Was there some limit reached (rule is u16 in rule_set.rs / contains()) maybe?

@charliermarsh
Copy link
Member

Try bumping the value in const RULESET_SIZE: usize = 13;

Copy link

codspeed-hq bot commented Apr 15, 2024

CodSpeed Performance Report

Merging #10963 will not alter performance

Comparing tibor-reiss:add-E0303-invalid-length-returned (b09ac0b) with main (b23414e)

Summary

✅ 30 untouched benchmarks

@tibor-reiss tibor-reiss force-pushed the add-E0303-invalid-length-returned branch 2 times, most recently from ee331b8 to 57fab81 Compare April 15, 2024 22:18
Copy link
Contributor

github-actions bot commented Apr 15, 2024

ruff-ecosystem results

Linter (stable)

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

python/typeshed (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --no-preview --select E,F,FA,I,PYI,RUF,UP,W

+ stdlib/typing.pyi:330:10: PYI001 Name of private `TypeVar` must start with `_`

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
PYI001 1 1 0 0 0

Linter (preview)

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

ibis-project/ibis (+1 -0 violations, +0 -0 fixes)

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

+ ibis/expr/types/relations.py:831:9: PLE0303 `__len__` does not return a non-negative integer

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

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

+ pandas/core/arrays/base.py:483:9: PLE0303 `__len__` does not return a non-negative integer
+ pandas/core/base.py:349:9: PLE0303 `__len__` does not return a non-negative integer

python/typeshed (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select E,F,FA,I,PYI,RUF,UP,W

+ stdlib/typing.pyi:330:10: PYI001 Name of private `TypeVar` must start with `_`

Changes by rule (2 rules affected)

code total + violation - violation + fix - fix
PLE0303 3 3 0 0 0
PYI001 1 1 0 0 0

@tibor-reiss
Copy link
Contributor Author

tibor-reiss commented Apr 16, 2024

Try bumping the value in const RULESET_SIZE: usize = 13;

@charliermarsh Is the performance drop of 4% also related to this please? The newly introduced rule is quite lightweight.

Solved, thanks @MichaReiser

@MichaReiser MichaReiser added the rule Implementing or modifying a lint rule label Apr 16, 2024
@@ -97,6 +97,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::InvalidBoolReturnType) {
pylint::rules::invalid_bool_return(checker, name, body);
}
if checker.enabled(Rule::InvalidLengthReturnType) {
pylint::rules::invalid_length_return(checker, name, body);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: You can pass the entire function definition instead of just passing the name and body

Suggested change
pylint::rules::invalid_length_return(checker, name, body);
pylint::rules::invalid_length_return(checker, function_def);

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 took the current bool and str cases as a blueprint, which passed also name and body. What would be the advantage of passing the function_def please? I would still need to reference name and body.

Comment on lines 85 to 88
let Expr::UnaryOp(ast::ExprUnaryOp { op, .. }) = value else {
return false;
};
matches!(op, ast::UnaryOp::USub)
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

Suggested change
let Expr::UnaryOp(ast::ExprUnaryOp { op, .. }) = value else {
return false;
};
matches!(op, ast::UnaryOp::USub)
matches!(
value,
Expr::UnaryOp(ast::ExprUnaryOp {
op: ast::UnaryOp::USub,
..
})
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done!

Comment on lines 65 to 78
if !matches!(
ResolvedPythonType::from(value),
ResolvedPythonType::Unknown
| ResolvedPythonType::Atom(PythonType::Number(NumberLike::Integer))
) || is_negative_integer(value)
{
Copy link
Member

Choose a reason for hiding this comment

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

It could make sense to change the order of the checks because is_negative_integer is relatively simple compared to ResolvedPythonType::from

Suggested change
if !matches!(
ResolvedPythonType::from(value),
ResolvedPythonType::Unknown
| ResolvedPythonType::Atom(PythonType::Number(NumberLike::Integer))
) || is_negative_integer(value)
{
if is_negative_integer(value) ||
!matches!(
ResolvedPythonType::from(value),
ResolvedPythonType::Unknown
| ResolvedPythonType::Atom(PythonType::Number(NumberLike::Integer))
)
{

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TLDR: changed.
I was not sure about which should be first because I would expect that there are more cases where the return type is wrong, compared to where the type is ok (int) but the sign not.

@MichaReiser
Copy link
Member

@tibor-reiss I bumped the index (and fixed the perf regression). You can rebase and we should then see the impact of your rule.

@tibor-reiss tibor-reiss force-pushed the add-E0303-invalid-length-returned branch from 57fab81 to 2b50cd0 Compare April 16, 2024 18:16
@charliermarsh charliermarsh changed the title [pylint] Implement invalid-length-returned (PLE0303) [pylint] Implement invalid-length-returned (E0303) Apr 18, 2024
@charliermarsh charliermarsh added the preview Related to preview mode features label Apr 18, 2024
@charliermarsh charliermarsh enabled auto-merge (squash) April 18, 2024 01:48
@charliermarsh charliermarsh merged commit 9f01ac3 into astral-sh:main Apr 18, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants