Open jaraco opened 6 months ago
Thanks for the clear write-up.
I worry about the behavior of mypy
here, the required placement of the suppression comment seems bizarre and inconsistent with suppression comments in other tools. I would be curious to see if they are receptive to supporting a suppression comment in either position (the earlier position applying to the full import
expression and the latter to the single line).
Regardless I agree we should not be breaking this. Without further investigation, I think it would be reasonable to mark the fix as unsafe if we detect suppression comments.
@zanieb I think we shouldn't look at this issue in isolation of isort
because the same happens if you format the above code (Playground). Adding special handling for isort only would limit our options when it comes to integrating isort
into the formatter.
For the formatter, pragma comments are handled differently in that they are excluded from the line-width computation. However, we decided not to implement any special placement logic for pragma comments (or suppress formatting) because a) the correct placement depends on the specific pragma comment, e.g. type: ignore
and a noqa
comment might both need a different placement, and b) disabling formatting for lines ending with pragma comments no longer delivers on the key value proposition, consistent formatting of your code (see https://github.com/astral-sh/ruff/discussions/6670).
I'm open to suggestions for improving pragma comment placement, but I fear moving some pragma comments manually might be necessary. This is inherent to the design decision to encode semantic meaning into comments with no grammar rules.
Possibly mypy requires the type: ignore
comment to be on the first line of the whole import statement due to the fact that ast.alias
nodes in Python's AST (as provided by the stdlib ast
module) only have lineno
and col_offset
attributes on 3.10+. So on older Python versions, mypy likely knows the lineno of the type: ignore
, and it knows the lineno of the start of the import statement as a whole, but it doesn't actually know the lineno of the specific import in the multiline import statement that it deems as problematic
I've run into this issue as well and figured I'd post my example in case helpful:
$ cat foo.py
from metaflow import batch # type: ignore
from metaflow import environment # type: ignore
from metaflow import resources # type: ignore
from metaflow import Flow, Run, S3, Step, Task, step
$ isort foo.py
$ python foo.py
$ mypy foo.py
Success: no issues found in 1 source file
$ ruff check --select I --fix foo.py
Found 1 error (1 fixed, 0 remaining).
$ cat foo.py
from metaflow import (
Flow,
Run,
S3,
Step,
Task,
batch, # type: ignore
environment, # type: ignore
resources, # type: ignore
step,
)
$ mypy foo.py
foo.py:1: error: Module "metaflow" has no attribute "batch" [attr-defined]
foo.py:1: error: Module "metaflow" has no attribute "environment" [attr-defined]
foo.py:1: error: Module "metaflow" has no attribute "resources" [attr-defined]
Found 3 errors in 1 file (checked 1 source file)
As indicated in the Issue description, removing the three specific # type: ignore
directives and adding one to the first line resolves the issue, albeit with slightly altered mypy
inspection.
I'm struggling to create a minimal example, so bear with this more complicated one:
In the keyring project, I'm exploring introducing the isort ("I") linter. When I add it and invoke it on the project, it produces a diff in the
keyring/_compat.py
module:After making that change, the mypy tests start failing:
Note that running mypy from another environment will not suffice - you need mypy in the environment where keyring was installed in order to get the right dependencies and stubs.
Moving the
# type: ignore[no-redef]
directive to the line above fixes the issue:Relatedly, note also that the coverage directive, which was previously not well placed, also now misses line 7 where it previously exempted it.
What I expect is that an isort
--fix
should retain the semantic meaning of directives in comments, or at least protect the rewrite in an "unsafe" fixer.