Skip to content

Conversation

@wilt00
Copy link

@wilt00 wilt00 commented Oct 26, 2025

Summary

This PR implements a new rule, nested-annotated-type, as discussed in #21037 . This rule fails for type definitions including Annotated nested within some other type.

Test Plan

New snapshosts in RUF066.py with cargo insta test.

@ntBre ntBre added rule Implementing or modifying a lint rule needs-decision Awaiting a decision from a maintainer labels Oct 27, 2025
@MichaReiser
Copy link
Member

MichaReiser commented Oct 30, 2025

I think this rule makes sense to me (catching Annotated[...] | ...) and I'd be in favor adding it because it catches a very likely mistake. But I'd be curious to hear @amyreese's opinion, too, just to ensure I'm not overlooking any valid use cases for this.

I do think we should come up with a less cryptic name :). We certainly can't use the nested terminology because the annotated documentation uses that to refer to Annotated[Annotated] nesting

Nested Annotated types are flattened. The order of the metadata elements starts with the innermost annotation:
https://docs.python.org/3/library/typing.html#typing.Annotated

Maybe something like type-operation-with-annotated or mixed-annotated-regular-type?

@amyreese
Copy link
Contributor

I agree that it's a useful rule — I'm not aware of any case where you would want to use an Annotated type with anything, let alone nest Annotated inside of anything other than another Annotated type.

For names I prefer mixed-annotated-regular-type over the other suggestion, but I might instead name it union-with-annotated-type

@MichaReiser MichaReiser removed the needs-decision Awaiting a decision from a maintainer label Oct 30, 2025
@wilt00 wilt00 changed the title [ruff] Implement nested-annotated-type (RUF066) [ruff] Implement union-with-annotated-type (RUF066) Nov 1, 2025
@MichaReiser MichaReiser requested a review from amyreese November 4, 2025 21:13
@ntBre
Copy link
Contributor

ntBre commented Nov 5, 2025

I'll leave the main review to @amyreese, but this looks reasonable overall to me. Would you mind re-numbering the rule to RUF068? We have existing PRs for RUF066 and RUF067.

@ntBre ntBre added the preview Related to preview mode features label Nov 5, 2025
def f3(z: Optional[Annotated[float, "A float"]]) -> None:
pass

e: Annotated[Annotated[int, "An integer"], "Another annotation"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This being an error implies that list[Annotated[...]] or similar would also produce an error. That part seems fine to me, but I wonder if it would be worth having a separate error message for these cases since the mention of union/optional doesn't make sense here? Would also like to have test cases showing that behavior one way or the other.

/// ```
///
#[derive(ViolationMetadata)]
#[violation_metadata(preview_since = "0.14.4")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs a bump as well 😅

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.

4 participants