Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

RUF012 false positive with annotations set as strings #12288

Open
ZeeD opened this issue Jul 11, 2024 · 8 comments
Open

RUF012 false positive with annotations set as strings #12288

ZeeD opened this issue Jul 11, 2024 · 8 comments
Labels
bug Something isn't working

Comments

@ZeeD
Copy link

ZeeD commented Jul 11, 2024

minimal case:

from typing import ClassVar
from typing import Final


class C:
    cv_orig: ClassVar[list[int]] = []
    cv_str: 'ClassVar[list[int]]' = []

    cv_final_orig: Final[list[int]] = []
    cv_final_str: 'Final[list[int]]' = []

    use_case: 'Final[list[C]]' = []

with ruff 0.5.1 I got the error

RUF012 Mutable class attributes should be annotated with `typing.ClassVar`

for cv_str, cv_final_str and use_case (that was my initial scenario...)

@dhruvmanila dhruvmanila added the bug Something isn't working label Jul 12, 2024
@dhruvmanila
Copy link
Member

I think this is the case for other rules as well like RUF008, RUF009, etc.

@charliermarsh
Copy link
Member

👍 Yeah we would need to parse these. I wonder if we should consider parsing these in the parser and adding them to the AST...

@charliermarsh
Copy link
Member

Actually, we probably can't do that because you often need semantic information to know if a string is a type annotation.

@dhruvmanila
Copy link
Member

We do parse it: https://github.com/astral-sh/ruff/blob/ecd6865d2800a574baf122583b6e463605af2ef5/crates/ruff_python_parser/src/typing.rs; I guess we need to provide some API to resolve it to the AST node if the model is in a type annotation context.

@charliermarsh
Copy link
Member

Yeah we just parse it much later, at the end IIRC.

@charliermarsh
Copy link
Member

Perhaps we could parse these on-demand and store them on the semantic model.

@charliermarsh
Copy link
Member

I guess that might not work because in theory these need to be evaluated lazily, at the very end (e.g., they can reference symbols that are imported afterwards, IIRC).

@dhruvmanila
Copy link
Member

At least for these 3 rules, they all require a class node which means we can store them in Analyze struct (

/// A collection of AST nodes to be analyzed after the AST traversal. Used to, e.g., store
/// all `for` loops, so that they can be analyzed after the entire AST has been visited.
#[derive(Debug, Default)]
pub(crate) struct Analyze {
pub(crate) scopes: Vec<ScopeId>,
pub(crate) lambdas: Vec<Snapshot>,
pub(crate) for_loops: Vec<Snapshot>,
}
) and then later use deferred_class.rs (similar to deferred_lambdas.rs) to check for violations for these rules. We might want to update existing logic to not raise a violation if the annotation is a string.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants