ruff
ruff copied to clipboard
astral-sh
Format Should Preserve Type Ignore Sub Expression
It seems that the ruff formatter will collapse a multi line expression with a # type: ignore to one line, unlike black.
I would like to keep the existing line split, if possible, since this limits the type ignore to one piece of the statement, instead of all of it. That way, if there is a type error in one of the other parts of the statement they are still caught by the type checker.
This is similar to https://github.com/astral-sh/ruff/issues/7730 but the issue here is not that the line ends up being too long, but that it changes the semantics of the type checker.
Example:
$ ruff format . --diff
--- python/egglog/egraph.py
+++ python/egglog/egraph.py
@@ -536,9 +536,7 @@
# If this is the class for this method and we have a paramaterized class, recurse
if (
- cls_type_and_name
- and isinstance(tp, _GenericAlias)
- and tp.__origin__ == cls_type_and_name[0] # type: ignore
+ cls_type_and_name and isinstance(tp, _GenericAlias) and tp.__origin__ == cls_type_and_name[0] # type: ignore
):
return TypeRefWithVars(
cls_type_and_name[1],
1 file would be reformatted, 25 files left unchanged
$ ruff --version
ruff 0.1.3
Settings:
[tool.ruff]
# Allow lines to be as long as 120.
line-length = 120
ignore = [
# allow star imports
"F405",
"F403",
# Dont care if cls isnt typed explicitly in classmethod
"ANN102",
# Same for self
"ANN101",
# Allow single line docstrings on multiple lines
"D200",
"D212",
# Inconsistant formatting
"D203",
"COM812",
"COM819",
"E501",
"ISC001",
"Q001",
"Q002",
"Q003",
"W191",
"Q000",
"D206",
# TODO: Remove the rest of these eventually
# Allow public methods to not have docstrings
"D102",
# Allow longer messages for custom errors
"TRY003",
# Allow f-string in exceptions
"EM102",
]
src = ["python"]
select = ["ALL"]
extend-exclude = ["python/tests"]
I agree this should remain split to narrow the type pragma, I'm surprised by this behavior.
Thanks for the well written issue! (I read this backwards at first so I modified the code highlighting to be a diff)
I can take a look at this though @MichaReiser will probably know the immediate answer. My guess is that the comment is attached to the overall binary expression rather than the tp.__origin__ == cls_type_and_name[0] term, so it doesn't cause the expression to expand.
Looks correct https://play.ruff.rs/b126b941-44e2-44bf-9cca-68370dc14b9c?secondary=Comments
I suspect this is actually quite hard to support given that you don’t always want to expand on trailing comments here, and you don’t want to expand even with a type ignore if the line is already collapsed.
My guess is that the comment is attached to the overall binary expression rather than the tp.origin == cls_type_and_name[0] term,
I think that's correct. We could refine how pragma comments get associated in binary expressions but I'm not sure how. I first thought of always assign the comment to the most inner operand but it doesn't necessary yield the desired result, e.g. the above would roughly be formatted as:
if (
cls_type_and_name
and isinstance(tp, _GenericAlias)
and tp.__origin__
== cls_type_and_name[0] # type: ignore
):
pass
We could try to be "clever" and also take the source formatting into consideration, by associating the type ignore comment with the outer most operand that is on the same line as the type ignore
A workaround is to use parentheses to guide the formatter, but I agree that this isn't ideal
if (
cls_type_and_name
and isinstance(tp, _GenericAlias)
and (
tp.__origin__ == cls_type_and_name[0] # type: ignore
)
):
pass
I feel like the ideal solution needs to somehow take into account which nodes are on the same line as the ignore. If we just assign to the inner-most operand, we'd have the opposite problem -- e.g., given cls_type_and_name and isinstance(tp, _GenericAlias) and tp.__origin__ == cls_type_and_name[0] # type: ignore, we'd end up breaking that over multiple lines.
Not sure if this is related but I'm having a similar issue inside list comprehension and long lines. Example:
todo: List[Optional[int]] = list(range(5))
result = [
- very_long_name_1234567 + 1 for very_long_name_1234567 in todo # type: ignore
+ very_long_name_1234567 + 1
+ for very_long_name_1234567 in todo # type: ignore
]
assert result == [0, 1, 2, 3, 4]
On pip we have a similar issue:
rmtree_errorhandler(
mock_func, path, sys.exc_info() # type: ignore[arg-type]
gets reformatted to
rmtree_errorhandler(
mock_func,
path,
sys.exc_info() # type: ignore[arg-type]
This causes mock_func and path to now get type-checked, when previously they would not.
IMO reformatting should never change semantics like this.
Hi @charliermarsh, any plans on this ?
IMO, it would be nice if ruff had the config option to skip formatting lines with #type: ignore
Hi @MaksimZayats
I'll reply briefly here, but I recommend you open a new discussion if you want to follow up to avoid going off-topic on this issue.
Thanks for the linked issue. The scenario outlined by Guido is something I considered when designing pragma comment support for ruff's formatter. What I understand from the linked issue is, and I agree with, is that it's annoying when the code gets reformatted after I had to add a type: ignore comment. That's why Ruff's formatter doesn't account pragma comments to the line width.
Unfortunately, this doesn't help if you reformat your code with ruff the first time. It can then happen that the type: ignore comment gets moved, e.g. because the formatter now splits a list of arguments onto multiple lines. I agree this isn't ideal because it requires manually moving the pragma comments to the right position. That's why we tried to design the handling in a way that tooling would at least warn you when a comment gets moved to avoid e.g. suppressing type checking for more code without you noticing (which could silence typing errors).
We also considered not to format lines with a type: ignore, but we considered this a bad trade-off because you lose out on the functionality of the formatter.
People use formatters to avoid discussing formatting decisions. Disabling formatting, or reduced formatting resurfaces the very same problem that people try to avoid by using a formatter. This isn't the same as us not caring about pragma comments or not wanting to use any pragma comment specific formatting. It only means that we don't want to use solutions that disable or limit formatting in a significant way.
I hope this is helpful and sets some of our decision-making into context.
Thank you @MichaReiser for the link to your design document regarding comment support! It seems very thorough.
After reading it, there are a few pieces in it that confuse me, since they seem to suggest that this issue wouldn't happen under those design constraints, although I am probably just missing something. It's quite a subtle issue!
When you discuss alternatives you discarded, I read the last sentence (emphasized by me) as suggesting that there is a way to manually move the type comment to preserve the intended semantics, i.e. that only one sub-expression should be ignored by the type checker, not the whole line. Is that how you meant it? If so, is there a way I can format the original example so that ruff will preserve those semantics?
- Disable line breaks for type: comments similar to Black [https://github.com/psf/black/pull/1040]. I understand its motivation but believe that limiting formatting for entire lines removes the advantage of using a formatter. Not counting pragma comments towards the line width balances "reducing the frustration when placing pragma comments" with "the benefits of using a formatter" better IMO. I admit that this approach doesn't solve the issue when you format a new project or extend the width of an existing line. But moving the comment manually is a reasonable ask in these situations.
In "Case 1: Suppress single argument/expression" you do give an example of how ruff preserves the scope of a pragma comment. I am realizing that the issue I raised here only occurs when the pragma happens to fall on the last sub-expression that could be reformated into a line. For example, ruff properly preserves the line breaks with this example:
if (
cls_type_and_name
and isinstance(tp, _GenericAlias)
and tp.__origin__ # type: ignore
== cls_type_and_name[0]
):
pass
However, if the pragma is on the last line, like in my original example, they are all reformated into one line:
if (
cls_type_and_name
and isinstance(tp, _GenericAlias)
and tp.__origin__
== cls_type_and_name[0] # type: ignore
):
pass
When you discuss alternatives you discarded, I read the last sentence (emphasized by me) as suggesting that there is a way to manually move the type comment to preserve the intended semantics, i.e. that only one sub-expression should be ignored by the type checker, not the whole line.
Yes, that's the intention of the last line.
However, if the pragma is on the last line, like in my original example, they are all reformated into one line:
I assume you're using a line-length of 120 or similar, in which case you get:
if (
cls_type_and_name and isinstance(tp, _GenericAlias) and tp.__origin__ == cls_type_and_name[0] # type: ignore
):
pass
The formatter doesn't allow you to narrow the type: ignore comment because it always collapses the line. The reason it collapse the comment this way is to make the operation reversible if a line expands:
a = (
cls_type_and_name and isinstance(tp, _GenericAlias) and tp.__origin__ == cls_type_and_naeeeeeeeme[0] # aaaaaaaaaaaaaa
)
becomes
a = (
cls_type_and_name
and isinstance(tp, _GenericAlias)
and tp.__origin__ == cls_type_and_naeeeeeeeme[0] # aaaaaaaaaaaaaa
)
and then collapse back to if it is shortened:
a = (
cls_type_and_name and isinstance(tp, _GenericAlias) and tp.__origin__ == cls_type_and_naeme[0] # aaaaaaaaaaaaaa
)
A possible solution is to change the comment association logic in placement.rs for binary like expression to associate any trailing comment with the last comparison (tp.__origin__ == cls_type_and_name[0]) instead of making it a trailing binary expression comment if the binary expression is formatted across multiple lines. However, it comes with the downside that splitting a binary expression with a trailing comment becomes nonreversible. I'm not sure if this is a good tradeoff.
We're seeing this when formatting a long import:
from stripe.request_options import RequestOptions as RequestOptionsStripeRequestOptions # type: ignore
becomes
from stripe.request_options import (
RequestOptions as RequestOptionsStripeRequestOptions,
) # type: ignore
which starts failing in type checking. +1 to the idea that formatting shouldn't cause CI failures.
It works if you keep the #ignore on the middle line, but i'm not sure that you could determine that programatically.
