[ruff] Detect duplicate entries in __all__ (RUF068)

[leandrobbraga]

Hello,

This MR adds a new rule and its fix, RUF069, DuplicateEntryInDunderAll. I'm using RUF069 because we already have RUF068 and RUF069 in the works.

The rule job is to prevent users from accidentally adding duplicate entries to __all__, which, for example, can result from copy-paste mistakes.

It deals with the following syntaxes:

__all__: list[str] = ["a", "a"]
__all__: typing.Any = ("a", "a")
__all__.extend(["a", "a"])
__all__ += ["a", "a"]

But it does not keep track of __all__ contents, meaning the following code snippet is a false negative:

class A: ...

__all__ = ["A"]
__all__.extend(["A"])

Violation Example

RUF069 `__all__` contains duplicate entries
 --> RUF069.py:2:17
  |
1 | __all__ = ["A", "A", "B"]
  |                 ^^^
help: Remove duplicate entries from `__all__`
1 | __all__ = ["A", "B"]
  - __all__ = ["A", "A", "B"]

Ecosystem Report

The ruff-ecosystem results contain seven violations in four projects, all of them seem like true positives, with one instance appearing to be an actual bug.

This code snippet from reportlab contains the same entry twice instead of exporting both functions.

def get_rl_tempdir(*subdirs: str) -> str: ...
def get_rl_tempfile(fn: str | None = None) -> str: ...

__all__ = ("get_rl_tempdir", "get_rl_tempdir")

Closes #21945

[astral-sh-bot]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

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

bokeh/bokeh (+1 -0 violations, +0 -0 fixes)

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

+ src/bokeh/core/query.py:53:5: RUF068 [*] `__all__` contains duplicate entries: `find` duplicated here

langchain-ai/langchain (+2 -0 violations, +0 -0 fixes)

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

+ libs/langchain/langchain_classic/document_loaders/__init__.py:373:5: RUF068 [*] `__all__` contains duplicate entries: `AcreomLoader` duplicated here
+ libs/langchain/langchain_classic/document_loaders/__init__.py:391:5: RUF068 [*] `__all__` contains duplicate entries: `AsyncHtmlLoader` duplicated here

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

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

+ stubs/convertdate/convertdate/__init__.pyi:45:5: RUF068 [*] `__all__` contains duplicate entries: `mayan` duplicated here
+ stubs/reportlab/reportlab/lib/rltempfile.pyi:4:30: RUF068 [*] `__all__` contains duplicate entries: `get_rl_tempdir` duplicated here
+ stubs/workalendar/workalendar/europe/__init__.pyi:252:5: RUF068 [*] `__all__` contains duplicate entries: `Switzerland` duplicated here

astropy/astropy (+1 -0 violations, +0 -0 fixes)

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

+ astropy/table/__init__.py:32:5: RUF068 [*] `__all__` contains duplicate entries: `conf` duplicated here

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
RUF068	7	7	0	0	0

[leandrobbraga]

It seems that those are all true positives.

This one might even be a bug.
https://github.com/python/typeshed/blob/06ecffccc72423711137c07be80c90522c084bbe/stubs/reportlab/reportlab/lib/rltempfile.pyi#L1-L4

[chirizxc]

What about these cases?

import typing


class A: ...


class B: ...

__all__ = "A" + "B"
__all__: list[str] = ["A", "B"]
__all__: typing.Any = ("A", "B")

[leandrobbraga]

What about these cases?

import typing


class A: ...


class B: ...

__all__ = "A" + "B"
__all__: list[str] = ["A", "B"]
__all__: typing.Any = ("A", "B")

Do you want to test whether those situations produce false positives or whether it still catches duplicates when using tuples, concatenation and/or type annotations?

[chirizxc]

What about these cases?

import typing


class A: ...


class B: ...

__all__ = "A" + "B"
__all__: list[str] = ["A", "B"]
__all__: typing.Any = ("A", "B")

Do you want to test whether those situations produce false positives or whether it still catches duplicates when using tuples, concatenation and/or type annotations?

I think it makes sense to add them to the test cases

[leandrobbraga]

What about these cases?

import typing


class A: ...


class B: ...

__all__ = "A" + "B"
__all__: list[str] = ["A", "B"]
__all__: typing.Any = ("A", "B")

Do you want to test whether those situations produce false positives or whether it still catches duplicates when using tuples, concatenation and/or type annotations?

I think it makes sense to add them to the test cases

Added these cases as suggested

[leandrobbraga]

I added some extra cases, based on RUF022:

__all__: list[str] = ["a", "a"]
__all__: typing.Any = ("a", "a")
__all__.extend(["a", "a"])
__all__ += ["a", "a"]

It still does not track mutable __all__, example:

__all__ = ["a"]
__all__ += ["a"]

This will be a false negative.

[chirizxc]

LGTM.

There may be other cases, but again, we cannot check dynamically created __all__, and we can only look at one file at a time, so if someone decides to add a duplicate in another file, we will not be able to detect it.

I also think it's worth adding RUF069.pyi.

[leandrobbraga]

LGTM.

There may be other cases, but again, we cannot check dynamically created __all__, and we can only look at one file at a time, so if someone decides to add a duplicate in another file, we will not be able to detect it.

I also think it's worth adding RUF069.pyi.

Done

[leandrobbraga]

Thank you! This is looking great. I just had a few more very small nits, and a couple of suggestions about additional tests (but I think they'll be handled well already).

I also think it might be worth manually testing the CLI to make sure cases with multiple deletions in the same assignment are fixed correctly. I think they should be, but we have a mechanism to isolate fixes if we need it. I think just testing something like:

$ cargo run -p ruff -- check --preview --select RUF069 --fix - <<<'__all__ = ["A", "A", "B", "B"]'

should work. If it doesn't work, we should add an actual CLI test, but I think it's okay as long as the manual test works as expected.

It does work:

$ cargo run -p ruff -- check --preview --select RUF069 --fix - <<<'__all__ = ["A", "A", "B", "B"]'
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 8.98s
     Running `target/debug/ruff check --preview --select RUF069 --fix -`
warning: Detected debug build without --no-cache.
__all__ = ["A", "B"]
Found 2 errors (2 fixed, 0 remaining).

I also have tested it in the crates/ruff_linter/resources/test/fixtures/ruff/RUF069.py file and the fix works flawless.

[leandrobbraga]

I should have addressed all concerns now, sorry for the delay

[chirizxc]

In general, it seems that we also have code that repeats in this rule. Should we move some functions to helpers and reuse them?

Do something like this:

pub(crate) fn all_assignment<F>(
    checker: &Checker,
    target: &ast::Expr,
    value: &ast::Expr,
    call: F,
) where
    F: FnOnce(&Checker, &[ast::Expr]),
{

    let ast::Expr::Name(ast::ExprName { id, .. }) = target else {
        return;
    };

    if id != "__all__" {
        return;
    }

    if !checker.semantic().current_scope().kind.is_module() {
        return
    }

    let elts = match value {
        ast::Expr::List(ast::ExprList { elts, .. }) => elts,
        ast::Expr::Tuple(ast::ExprTuple { elts, .. }) => elts,
        _ => return,
    };

    call(checker, elts);
}

[chirizxc]

It is possible to leave it as it is. However, if we decide to add more checks to the rule in the future, for example, to track .append(), we will have to change the code, and moving it to helpers may not be that useful.

[leandrobbraga]

In general, it seems that we also have code that repeats in this rule. Should we move some functions to helpers and reuse them?

Do something like this:

pub(crate) fn all_assignment<F>(
    checker: &Checker,
    target: &ast::Expr,
    value: &ast::Expr,
    call: F,
) where
    F: FnOnce(&Checker, &[ast::Expr]),
{

    let ast::Expr::Name(ast::ExprName { id, .. }) = target else {
        return;
    };

    if id != "__all__" {
        return;
    }

    if !checker.semantic().current_scope().kind.is_module() {
        return
    }

    let elts = match value {
        ast::Expr::List(ast::ExprList { elts, .. }) => elts,
        ast::Expr::Tuple(ast::ExprTuple { elts, .. }) => elts,
        _ => return,
    };

    call(checker, elts);
}

I guess we can defer this to later and have an specific MR for refactors.

[ntBre]

Yeah I think this is probably okay. I'll try to take another look tomorrow!

[ntBre]

Very nice, thank you!

I pushed a couple of commits applying my last test suggestion and re-coding the rule to avoid a gap at RUF068, but this is great.

[ntBre]

I'll open a follow-up issue for this since it's not related to your PR, but this shouldn't always be a safe fix either:

❯ cat <<EOF | ruff check --diff --select B033 -
∙ {1, 2, 3,
# comment
1}
∙ EOF
@@ -1,3 +1 @@
-{1, 2, 3,
-# comment
-1}
+{1, 2, 3}

Would fix 1 error.