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)

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

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

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

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

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

What about these cases?

import typing


class A: ...


class B: ...

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

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?

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

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

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.

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.

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

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.

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

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);
}

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.

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.

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