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

RUF031: Ignore unparenthesized tuples in subscripts when the subscript is obviously a type annotation or type alias #12762

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

AlexWaygood
Copy link
Member

Summary

Fixes #12758. Previously with the setting lint.ruff.parenthesize-tuple-in-subscript = true, RUF031 would have complained about annotations such as dict[str, int], telling you that you should spell them as dict[(str, int)] instead. That feels somewhat silly -- although it is of course a subscription in exactly the same way as a dictionary key lookup, it's not really what the rule is trying to detect.

This PR fixes most of those cases. Unfortunately it doesn't fix cases like this, where it's ambiguous whether the variable is a type alias or not:

import typing
X = typing.Callable[[str, int], bool]  # RUF031 here

Faced with this, however, users have a fairly simple workaround: make it explicit that it's a type alias, and the false positive will go away

import typing
X: typing.TypeAlias = typing.Callable[[str, int], bool]

Test Plan

cargo test -p ruff_linter --lib

@AlexWaygood AlexWaygood added bug Something isn't working rule Implementing or modifying a lint rule preview Related to preview mode features labels Aug 8, 2024
Copy link
Contributor

github-actions bot commented Aug 8, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

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

RasaHQ/rasa (+0 -1 violations, +0 -0 fixes)

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

- tests/utils/test_train_utils.py:117:67: RUF031 [*] Avoid parentheses for tuples in subscripts.

Changes by rule (1 rules affected)

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

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Aug 8, 2024

RasaHQ/rasa (+0 -1 violations, +0 -0 fixes)

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

Oh, that's ugly. It sorta feels like a shame that we'll no longer be emitting a diagnostic there 😆

I suppose we could only ignore the rule for type annotations/aliases if lint.ruff.parenthesize-tuple-in-subscript = true, and continue to apply the rule everywhere if lint.ruff.parenthesize-tuple-in-subscript = false? But that feels conceptually harder to explain

@dylwil3
Copy link
Contributor

dylwil3 commented Aug 8, 2024

Does it make sense to say in the docstring for the rule that we skip annotations, or is that too in the weeds?

@AlexWaygood
Copy link
Member Author

That's a good idea. I'll add some more docs tomorrow (it's late now where I am :-)

@@ -64,6 +64,10 @@ pub(crate) fn subscript_with_parenthesized_tuple(checker: &mut Checker, subscrip
if tuple_subscript.parenthesized == prefer_parentheses || tuple_subscript.elts.is_empty() {
return;
}
let semantic = checker.semantic();
if semantic.in_annotation() || semantic.in_type_definition() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also do semantic.in_simple_string_type_definition() here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not needed -- if the SIMPLE_STRING_TYPE_DEFINITION flag has been set on the semantic model, the TYPE_DEFINITION flag will also have been set:

let type_definition_flag = match kind {
AnnotationKind::Simple => SemanticModelFlags::SIMPLE_STRING_TYPE_DEFINITION,
AnnotationKind::Complex => {
SemanticModelFlags::COMPLEX_STRING_TYPE_DEFINITION
}
};
self.semantic.flags |=
SemanticModelFlags::TYPE_DEFINITION | type_definition_flag;

@dhruvmanila
Copy link
Member

I think it might make sense to cover generics as well (#12773 (comment)) like:

class Foo(Generic[T1, T2]):
	...

(It can be done as a follow-up.)

@MichaReiser
Copy link
Member

such as dict[str, int], telling you that you should spell them as dict[(str, int)] instead. That feels somewhat silly -- although it is of course a subscription in exactly the same way as a dictionary key lookup, it's not really what the rule is trying to detect.

I find it hard to following this reasoning but it's probably just me not writing enough python. Isn't this as "silly" as doing the same in regular subscripts?

…ipt is obviously a type annotation or type alias
@AlexWaygood
Copy link
Member Author

I find it hard to following this reasoning but it's probably just me not writing enough python. Isn't this as "silly" as doing the same in regular subscripts?

Type annotations just have their own specific conventions around them. And the subscription has a very different meaning in a typing context. You're not "looking up" a value at an index or key in a typing context in the same way; and I think the vast majority of people aren't even aware that they're creating implicit tuples in a type annotation context.

@AlexWaygood AlexWaygood merged commit 83db48d into main Aug 9, 2024
19 checks passed
@AlexWaygood AlexWaygood deleted the alex/annotation-tuples branch August 9, 2024 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 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.

RUF031 with parenthesize-tuple-in-subscript = true should ignore type annotations
4 participants