aspect-build / rules_lint

Run static analysis tools with Bazel
Apache License 2.0
66 stars 39 forks source link

[Bug]: lint_ruff_aspect does not fully support sorting imports #303

Open honnix opened 3 weeks ago

honnix commented 3 weeks ago

What happened?

When enabling sorting imports in e.g. pyproject.toml, there are cases ruff cannot categorize an import to be StandardLibrary, FirstParty, or ThirdParty, because it does not see a full source tree.

Version

Development (host) and target OS/architectures:

Output of bazel --version: bazel 7.2.1

Version of the Aspect rules, or other relevant rules from your WORKSPACE or MODULE.bazel file: v1.0.0-rc4

Language(s) and/or frameworks involved: Python

How to reproduce

Consider a simple source tree as the following:

src
|_foo.py
|_foo_test.py

where foo_test.py has imports like:

import known_third_party
import foo

pyproject.toml:

[tool.ruff]
src = ["src"]

[tool.ruff.lint]
extend-select = ["I"]

And the following bazel stuff:

py_binary(
    name = "foo",
    srcs = ["foo.py"],
)

py_test(
    name = "foo_test",
    srcs = ["foo_test.py"],
    deps = [":foo"]
)

ruff = lint_ruff_aspect(
    binary = "@@//path/to:ruff",
    configs = ["@@//:pyproject.toml"],
)

ruff_test = lint_test(aspect = ruff)

ruff_test(
    name = "ruff_test",
    srcs = [":foo_test"],
)

bazel test //path/to:ruff_test would fail because ruff cannot find foo.py so it categorized it as a ThirdParty, which means it wants this instead:

import foo
import known_third_party

Any other information?

After a closer look at this, it seems lint_ruff_aspect only populates srcs to be in the sandbox source tree, which means only src/foo_test.py is in the source tree, not deps.

It also doesn't help having:

ruff_test(
    name = "ruff_test",
    srcs = [":foo", ":foo_test"],
)

because it would just be two separated ruff invocations, one with foo.py in source tree and the other one with foo_test.py in source tree.

Another thing to point out is, lint_ruff_aspect does not support having pyproject.toml not in project root. I left a comment here.

alexeagle commented 3 weeks ago

It's easy enough to pass ruff the transitive sources as inputs. I'm curious if you know of other cases where they are needed? The downside is that a file edit will now invalidate lint actions for all reachable nodes in the graph, making it slower.

honnix commented 3 weeks ago

I'm not sure of other cases.

The downside is that a file edit will now invalidate lint actions for all reachable nodes in the graph, making it slower.

I don't fully understand this. Could you please elaborate a bit more? Thanks.

alexeagle commented 2 weeks ago

If app/main.py imports from lib/helper.py, then edits to helper.py will now invalidate the linting for the py_library or py_binary in the app/ folder now, where previously it wouldn't. That's potentially okay - it's required for type-checkers to behave this way. But, if it's only to support a few sorted imports appearing differently, then it's too bad to burn engineers time and compute resources on a lot of re-linting when a base library changes.

honnix commented 2 weeks ago

Thank you for explaining. And yeah I agree with the assessment.