Skip to content

Add rule to detect unnecessary class properties #21535

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.

Sign up for GitHub

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

Jump to bottom
Merged
MichaReiser merged 22 commits into from
Nov 26, 2025
Merged

Add rule to detect unnecessary class properties #21535

MichaReiser merged 22 commits into from
Nov 26, 2025

Conversation

@ShaharNaveh
Copy link
Contributor

@ShaharNaveh ShaharNaveh commented Nov 20, 2025

Summary

Fixes #21403

Test Plan

Added tests

@astral-sh-bot
Copy link

astral-sh-bot bot commented Nov 20, 2025 •
edited
Loading

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

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

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

ruff check --no-cache --exit-zero --no-fix --output-format concise --preview

+ pandas/tests/io/excel/test_writers.py:1708:17: RUF069 `sheets` is a property without a `return` statement

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

ruff check --no-cache --exit-zero --no-fix --output-format concise --preview

+ testing/python/raises.py:346:17: RUF069 `__class__` is a property without a `return` statement
+ testing/test_compat.py:117:13: RUF069 `__class__` is a property without a `return` statement
+ testing/test_compat.py:91:9: RUF069 `raise_fail_outcome` is a property without a `return` statement

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
RUF069 4 4 0 0 0

@ShaharNaveh ShaharNaveh marked this pull request as draft November 20, 2025 09:10
@ShaharNaveh ShaharNaveh changed the title Add rule to detect class properties without a return statement Add rule to detect unnecessary class properties Nov 20, 2025
@ShaharNaveh ShaharNaveh marked this pull request as ready for review November 20, 2025 11:39
@amyreese amyreese added the rule Implementing or modifying a lint rule label Nov 20, 2025
ntBre
ntBre requested changes Nov 20, 2025
View reviewed changes
Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good to me overall, just some minor nits and a suggestion about the rule name and code.

crates/ruff_linter/src/rules/ruff/rules/unnecessary_property.rs Outdated
use crate::{FixAvailability, Violation};

/// ## What it does
/// Detects unnecessary `@property` methods.
Copy link
Contributor

@ntBre ntBre Nov 20, 2025

Choose a reason for hiding this comment

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

"Unnecessary" doesn't feel like exactly the right description to me. I think something like property-without-return for the rule name, along the lines of the issue title, might make more sense.

Copy link
Contributor Author

@ShaharNaveh ShaharNaveh Nov 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The initial name was something like that, but I decided it's no longer accurate after the inclusion of raise and yield, would you still want me to revert it to the old name?

Copy link
Contributor

@ntBre ntBre Nov 21, 2025

Choose a reason for hiding this comment

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

Ah, I see what you mean, but I think it's still okay for the name and diagnostic message only to mention return. @amyreese had a good point that return will probably be the most common fix when the diagnostic fires too, so it seems good to emphasize that option.

@ntBre ntBre added the preview Related to preview mode features label Nov 20, 2025
@ShaharNaveh ShaharNaveh requested a review from ntBre November 21, 2025 09:44
ntBre
ntBre requested changes Nov 21, 2025
View reviewed changes
Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

Thank you, this looks great!

I think we just need to bump the preview_since version and update the rule name/docs and this is good to go.

crates/ruff_linter/src/rules/ruff/rules/unnecessary_property.rs Outdated
use crate::{FixAvailability, Violation};

/// ## What it does
/// Detects unnecessary `@property` methods.
Copy link
Contributor

@ntBre ntBre Nov 21, 2025

Choose a reason for hiding this comment

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

Ah, I see what you mean, but I think it's still okay for the name and diagnostic message only to mention return. @amyreese had a good point that return will probably be the most common fix when the diagnostic fires too, so it seems good to emphasize that option.

amyreese
amyreese requested changes Nov 24, 2025
View reviewed changes
crates/ruff_linter/src/rules/ruff/rules/property_without_return.rs Outdated
PropertyWithoutReturn {
name: name.to_string(),
},
function_def.range(),
Copy link
Member

@amyreese amyreese Nov 24, 2025

Choose a reason for hiding this comment

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

Sorry to add one more change needed. We should pick a smaller range for the diagnostic, rather than throwing a yellow squiggle on an entire function in an editor, as changing the diagnostic range later will potentially be a breaking change.

I think I'd suggest just marking the @property decorator itself, since that's the part that's unfulfilled by the body of the function.

Copy link
Member

@MichaReiser MichaReiser Nov 24, 2025

Choose a reason for hiding this comment

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

In some cases, we highlight the range of the function's name. You can see other usages by searching for references to the Identifier trait

ruff/crates/ruff_python_ast/src/identifier.rs

Lines 18 to 21 in 9ae698f

pub trait Identifier {
/// Return the [`TextRange`] of the identifier in the given AST node.
fn identifier(&self) -> TextRange;
}

Copy link
Contributor Author

@ShaharNaveh ShaharNaveh Nov 25, 2025

Choose a reason for hiding this comment

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

Thank you @amyreese & @MichaReiser for the suggestions.

I agree with highlighting only one line instead of the entire function block.

I ended up highlighting only the function name (@MichaReiser suggestion), just because it's a personal preference of mine. LMK if you want me to change that to highlight something else instead

amyreese
amyreese approved these changes Nov 25, 2025
View reviewed changes
Copy link
Member

@amyreese amyreese left a comment

Choose a reason for hiding this comment

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

One last nit, but otherwise looks good to me!

@ShaharNaveh @amyreese
Update crates/ruff_linter/src/rules/ruff/rules/property_without_retur…
2365137 
…n.rs

Co-authored-by: Amethyst Reese <amethyst@n7.gg>
@ShaharNaveh ShaharNaveh requested a review from amyreese November 26, 2025 07:55
@MichaReiser
Copy link
Member

MichaReiser commented Nov 26, 2025 •
edited
Loading

Thanks this looks good. I just noticed that you recoded the rule from RUF066 to RUF069 before. Can you tell me why? It seems to me that RUF066 is still available.

@MichaReiser
Copy link
Member

MichaReiser commented Nov 26, 2025

Oh, never mind. It's because there are other in-flight PRs using that code. I recoded it back to RUF066, because I want to avoid holes and your PR is going to land first :)

@MichaReiser MichaReiser requested review from ntBre and removed request for ntBre November 26, 2025 08:30
@MichaReiser MichaReiser merged commit 33713a7 into astral-sh:main Nov 26, 2025
36 checks passed
@BrewTestBot BrewTestBot mentioned this pull request Nov 28, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaReiser MichaReiser MichaReiser approved these changes

@amyreese amyreese Awaiting requested review from amyreese

@ntBre ntBre Awaiting requested review from ntBre

Assignees

No one assigned

Labels

preview Related to preview mode features rule Implementing or modifying a lint rule

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Class property without return

4 participants

@ShaharNaveh @MichaReiser @amyreese @ntBre