[flake8-type-checking] Add a fix for runtime-string-union (TC010)
#15222
Conversation
|
As a side note: I'm not quite sure why some of these fixes are marked as unsafe, even though the union expression doesn't overlap a comment. Do we perhaps have a off-by-one error here? |
|
| /// Traverses the type expression and checks if the expression can safely | ||
| /// be unquoted | ||
| fn quotes_are_removable(semantic: &SemanticModel, expr: &Expr, settings: &LinterSettings) -> bool { | ||
| match expr { | ||
| Expr::BinOp(ast::ExprBinOp { | ||
| left, right, op, .. | ||
| }) => { | ||
| match op { | ||
| Operator::BitOr => { | ||
| if settings.target_version < PythonVersion::Py310 { | ||
| return false; | ||
| } | ||
| quotes_are_removable(semantic, left, settings) | ||
| && quotes_are_removable(semantic, right, settings) | ||
| } | ||
| // for now we'll treat uses of other operators as unremovable quotes | ||
| // since that would make it an invalid type expression anyways. We skip | ||
| // walking subscript | ||
| _ => false, | ||
| } | ||
| } | ||
| Expr::Starred(ast::ExprStarred { | ||
| value, | ||
| ctx: ExprContext::Load, | ||
| .. | ||
| }) => quotes_are_removable(semantic, value, settings), | ||
| // Subscript or attribute accesses that are valid type expressions may fail | ||
| // at runtime, so we have to assume that they do, to keep code working. | ||
| Expr::Subscript(_) | Expr::Attribute(_) => false, | ||
| Expr::List(ast::ExprList { elts, .. }) | Expr::Tuple(ast::ExprTuple { elts, .. }) => { | ||
| for elt in elts { | ||
| if !quotes_are_removable(semantic, elt, settings) { | ||
| return false; | ||
| } | ||
| } | ||
| true | ||
| } | ||
| Expr::Name(name) => { | ||
| semantic.lookup_symbol(name.id.as_str()).is_none() | ||
| || semantic | ||
| .simulate_runtime_load(name, TypingOnlyBindingsStatus::Disallowed) | ||
| .is_some() | ||
| } | ||
| _ => true, | ||
| } | ||
| } |
There was a problem hiding this comment.
While there is some code duplication between this and quotes_are_unremovable, this version is simplified for lookups from a runtime context and needs to use lookup_symbol instead of resolve_name since the forward reference has not been traversed yet by the checker.
So I'm torn about whether to merge this with quotes_are_unremovable into a common function in helpers.rs and leveraging an enum with two variants to determine whether we can use resolve_name or need to use lookup_symbol instead. Alternatively we could try passing in a closure. But both approaches seem kind of messy to me.
We could also say we're fine with the additional overhead of lookup_symbol over resolve_name and just always use that.
| Expr::StringLiteral(_) => { | ||
| strings.push(expr); | ||
| Expr::StringLiteral(literal) => { | ||
| if let Ok(result) = parse_type_annotation(literal, locator.contents()) { |
There was a problem hiding this comment.
I'm not stoked about having to do this and not taking advantage of the cache on the checker, but I don't see a good way to avoid this without moving analysis further down the pipeline when traversing the parsed forward reference and checking for a union in the parent expression.
The only problem with that is that we would potentially generate the same fix for neighboring violations, without them being explicitly grouped together, but maybe we can get around that by setting the fix's parent to the location of the expression we're extending the quotes to.
The other issue with moving the analysis to a different stage is that we're potentially destabilizing a stable rule in doing so and potentially emitting incorrect violations in nested forward references like A | "B | 'C'", unless we add additional semantic state so we can detect if we're inside a nested forward reference.
That being said, it still might be the correct decision, in order to reduce some of the complexity.
There was a problem hiding this comment.
If we go with changing where the analysis happens, we at least have a straightforward way to keep the old implementation while the new one is in preview. Each version of the function can just check whether preview is enabled or not.
We may want to only enable the fix in preview anyways, regardless of how we choose to implement this.
|
@MichaReiser @AlexWaygood I'm taking this out of draft. Depending on your responses to my review comments this may either only need small changes or larger changes where experimenting in a separate PR would make more sense, so we can compare the two approaches. |
Summary
This adds a fix for
TC010which removes quotes whenever they can be removed safely and otherwise extends the quotes to the entire union.This also disables
TC010explicitly in stubs, rather than rely on the execution context, which can still be runtime in stub files for a small set of type expressions.Test Plan
cargo nextest run