TC006 in 0.8.1 is too broad
#14676
Comments
|
@Daverball, I'm curious for your thoughts here. The rule's docs say:
That implies that the concern is that a lot of time might be spent evaluating subscript expressions for generics in things like def f(y):
x = cast(Foo[str], y)It's true that evaluating the subscript expression will take the same amount of time for a first-party generic defined in the same module as it will for a third-party generic imported from another file. However, I feel like the specialising interpreter on Python 3.11+ is probably very good at making sure that repeated subscripts and attribute expressions inside
One common reason that people quote special types from the typing module is that it means that you can sometimes get rid of all imports from typing altogether from a module, which can speed up that module's import time. That doesn't apply here, though, since we know that the user is using |
AlexWaygood
changed the title
TC006 fix in 0.8.1 is too broadTC006 in 0.8.1 is too broad
|
Thanks for the quick response @AlexWaygood I only had one question regarding:
@AlexWaygood would this still apply when you'd need to import |
There's a little hack you can do for mypy where you can replace |
|
But yeah, if you have one import from |
|
No matter what you do, a quoted expression will always be fastest since, even if it's just one symbol you're getting rid of, the name lookup is more expensive than just putting a static value on the stack, while the optimizing compiler may alleviate this to a degree, where in the best case scenario this will eventually be just as fast once the code has been running for a while, you're still making the compiler work for that gain. So my take is that there's no good reason to keep the argument unquoted. There are certainly cases where the positive impact of quoting is low enough, that you could say it doesn't really matter, but I'd rather be consistent and always quote the expression than end up with a wishy washy mix, where you have to explain to people why sometimes there are quotes and sometimes there are not. So my take is, if you don't like the aesthetics and don't mind the overhead, then disable the rule. I would be open to adding additional settings to make this rule more flexible, so you can keep the expression unquoted in more cases, but I would personally turn all of those off in my code. In my mind this, next to TC005, is the least controversial rule in |
|
But if someone wants to provide a better explanation of what this rule does and why, please feel free to open a pull request, or even just suggest better phrasing in here and I will do it for you. I admittedly did not spend a lot of time on writing the docs for this rule, because of how straightforward it is (i.e. there are no exceptions, all I have been using it for quite a while through the flake8 plugin and it is very obvious to me, why it makes sense. But I can see how it could be less obvious to someone else. Especially if the initial gut reaction is, that it makes the code less pretty. |
Right, I'm not disputing that this rule gets rid of some overhead at runtime. I'm just disputing whether the (probably quite marginal) performance gain is worth it in all these cases considering a somewhat significant cost to readability (in my opinion). As PEP 20 reminds us, readability counts :-)
I could live with making the rule configurable, but adding too many configuration options to Ruff also comes with a cost. We already have so many configuration options that it's somewhat bewildering to users. I'm curious for other people's opinions here -- maybe @carljm? |
That's fair, although that only applies to syntax highlighted code. And syntax highlighters could easily detect with some simple heuristics most of the forward references and provide inline syntax highlighting for them, at which point the readability concerns would vanish. FWIW I'm not really happy that Python chose to use strings for forward references (or reuse subscripts for generics) and I don't like how they look either. So that choice has come back to haunt us in myriad of different ways, where a dedicated syntax for type expressions and forward references could have allowed us a lot more flexibility, so we wouldn't have to choose between aesthetics and unwarranted runtime overhead. But I also recognize that it would have been difficult to get the optional static type system off the ground at all, if it involved introducing additional new syntax, so it is what it is. Everyone will have to make their own trade-offs based on their use-cases, But I can at least tell you that while, when this rule was first introduced in |
|
I just hit this issue by upgrading 0.8.1. Took me a bit of time to manually fix the few occurrences where things could not be automatically fixed but nothing too long. I don't think readability suffered, and even a tiny bit of performance increase by removing something from runtime sounds good to me. But yes at first I thought something is off, until I read the rule's description and the discussion here. So my first reaction was "this may be a bug?". |
|
Count me as one of the very happy historic users of the flake8 plugin option for enforcing that all cast calls are quoted. I'm not sure I understand the appeal of avoiding this from a syntax highlighting perspective. I don't consider type-based syntax highlighting as a function call parameter to be beneficial to readability. Strings are more visually distinctive, and in a way that lets me mentally skip over them when checking a line of code to see what the effective code is doing. I'd probably enable this rule even if there was no runtime overhead or imports to be concerned about, just for the readability improvements. |
|
@Daverball appreciate your detailed responses. I'm fine with the readability post-fix, I use I'm happy with Thanks again @Daverball , @AlexWaygood |
|
It's clear to me from this discussion that there are lots of people who are used to the behaviour of the flake8 plugin and find it both useful and desirable. That's high enough signal for me that I think we should at least allow users to opt into having Ruff's current behaviour, where it will tell you to stringify the first argument of all calls using I'm still not convinced that this should be the default behaviour without any configuration, however. I feel like we already have too many rules that propose changes which, while they might be desirable in the abstract, offer only very minor improvements over the user's existing code. Having lots of rules like this means that running Ruff becomes more of a burden than a help for end users; you have to review a large quantity of changes with each Ruff minor upgrade, and for little gain. Autofixes help here, but they don't solve the problem, because the change an autofix makes still needs to be manually reviewed. My feeling is that stringifications that wouldn't result in imports being put behind
|
|
My opinion here is that the pros and cons are so small that it's really not worth adding either configurability or any more complexity to the rule. I think that will just lead to more confusion, and I just don't think it's worth the implementation or maintenance cost. I would favor keeping the rule simple and straightforward, as it is. If you like it, use it, if you don't, don't. I don't see evidence in this thread of user demand for a more complex version of the rule. (I do see a case for adding a line to the docs to clarify that the rule prioritizes consistency, even though some type expressions have very little or no cost.) I certainly wouldn't include this rule in the default rule-set, though. People should only see this rule if they opt into it, or if they choose to opt in to ALL. |
I've been trying to have a bit more empathy for people opting into ALL after seeing astropy/astropy#17430 (comment) 😄 Is it sensible to opt into ALL if you mainly want rules that you can have high confidence in, and you're not a fan of having large amounts of churn in your codebase with each Ruff minor release? No, probably not, at least with our current categorisation. People with those concerns still opt into ALL, however, in part due to the other limitations of our current categorisation. |
|
In fact I don't consider it sensible to both:
For example, let's say you have an existing rule that does one particular thing, and then a version update does the same thing but detects it in more places. Do you rejoice that you caught more code defects? Do you complain that the tool keeps changing? How can this problem be solved save by just sticking with a version and never upgrading? If one does feel that the tool keeps changing in ways that decrease satisfaction, I can't help but feel the root cause of the dissatisfaction is a matter of disagreement about what constitutes "broadly beneficial", rather than a more generalized dislike of "change". The solution is probably to implement #12111 and let people freeze their config at a version of ruff which they like, and then manually curate rules after that. On the other hand I think it would be reasonable to express discontent at the upstream tool changing how it works, such that one version of the tool makes one modification to your own codebase, and a new version of the tool changes course and modifies your code in a different way. That crosses the line from "the tool became better at doing what I asked it to do" to "the tool is inconsistent about what it wants". (This is actually one of several reasons why I strongly oppose autoformatters.) |
|
It's also worth mentioning that there's already some overlap with So it seems more productive to give people that currently opt into I think |
The challenge with this approach is that it becomes very unlikely that Leaving the rule as is seems fine to me if its main and only benefit is enforcing a specific coding style, but that's not what I understand is the main reason for the rule's existence. TLDR: The problem I see with how the rule is defined today is that it mixes stylistic and performance concerns, making it difficult to recategorize the rule in the future. |
|
@MichaReiser As I've previously pointed out, other use-cases can partially already be served by TC004 and should eventually be serviceable through the rest of I don't think this entire plugin can ever make it into a recommended or strict ruleset (apart from a few uncontroversial exceptions like TC004, TC005 and TC010), since it requires configuration in order to play nicely with libraries that make use of a subset of the type annotations at runtime like pydantic or SQLAlchemy. I'm fine with changing the docs to focus on style/consistency rather than performance. But I think you'll find categorizing It's unfortunately the nature of the beast where varying concerns push the preferred style in vastly different directions and that style choice is usually not made purely for aesthetics or readability. |
|
@MichaReiser What might make more sense to me is to add a new rule that focuses purely on the performance aspect of the creation of unnecessary So this would apply to more than just |
That works well for a flake8 plugin where, as the plugin author, you intend all the rules to be either selected together or not selected. There's a similar interaction between the different rules in a plugin I maintain, flake8-pyi. It's a bad fit for Ruff, however, where we need to consider the merits of each rule in isolation, and where we cannot (and should not) force users to either select all rules from a certain category or none of them. As such, several PYI rules work somewhat differently in Ruff than they do in the original flake8 plugin, in order to make them less interdependent and work better in isolation. Sometimes this is frustrating from an implementation perspective (#14583 would have been unnecessary if we could force users to always enable PYI016 if they enabled PYI061), but I believe that overall it results in a better experience for Ruff users. |
@Daverball are you sure there's overlap? I tried running from typing import cast
from expensive import Foo
x = cast(Foo, "foo")and The only flake8 error that was emitted was TC006. Same for Ruff with TC selected and |
|
@AlexWaygood Absolutely, but that's not what I meant by that statement. So the scope and the behavior of the rules can and will still drastically change according to the configuration, otherwise we will provide an objectively worse experience, where you can't realistically combine some of the rules without also adjusting the configuration. So it's still difficult to put individual rules in a single category, unless that category is The only other way to get around that is to explode the number of rules and make some of them mutually exclusive, in order to provide the same number of possibilities, but each of them having a clearly distinct purpose, but that seems even more confusing to me.
If you move the import to a Also I would consider that behavior a bug in the implementation of TC004. We also should probably not emit a |
but I also demonstrated that there doesn't seem to be any overlap when you use flake8 with the flake8-type-checking plugin. The flake8-type-checking plugin only seems to emit TC006 on that snippet. What am I missing? |
|
Oh sorry, I missed that part of the question. Yes, that's one of the major design differences between the flake8 plugin and ruff. In the flake8 plugin there are separate rules for adding quotes to annotations, so TC004 would not really be relevant there, but rather TC200, but that rule explicitly only covers annotations and not |
|
Okay. I'm still not sure I fully understand which yet-to-be-implemented rule you expect to cover the performance aspects of this rule. But if you're confident that it will be covered by another rule, then I agree that this rule should not be made configurable, and instead the docs should be reworked to make clear that it is an opinionated rule solely concerned with style. |
|
The import overhead can be covered by TC001-003 by expanding Similar questions come up for TC007, which could technically also partially be rolled into TC001-003 by adding a The subscript overhead could be covered by a more targeted rule that would also target In either case TC006 should take precedence over TC001-003 and this potential new rule. Then there's an entirely different topic covered by the other missing rules about removing redundant quotes, where ruff currently only provides partial support through UP037 for the |
There is a long discussion in <astral-sh/ruff#14676> about how useful this rule is, especially the performance vs. syntax highlighting question. Note that apparently `cast()` is not affected by `from __future__ import annotations`; adding it won't help. I'm kind of indifferent as to whether this is a good rule or not, therefore I'll accept ruff's recommendation for now. Signed-off-by: Tim Weber <scy@scy.name>
I was surprised by some of these "fixes" adding quotes
The reasoning in why-is-this-bad makes me think the concern is regarding parameterizing a generic, or maybe time spent by
ruff/ a type checker to see if the symbol is available at runtime?diff (
altair)I'm unsure the new tests sufficiently cover some of these cases:
builtinswith no free type variables likestrtypingspecial types likeAnyAlternative
Update the docs for
TC006to give more detail on how cases like these would benefit from the fix.I'm a big fan of
ruffand feel like I've learned a lot from reading the rules; so I'm open to this simply being something that hasn't clicked for me yetThe text was updated successfully, but these errors were encountered: