[pyupgrade] [ruff] Don't apply renamings if the new name is shadowed in a scope of one of the references to the binding (UP049, RUF052) - #16032
Conversation
|
|
@ntBre class C[_T]:
def f[T](self):
a: _T = ...The new logic introduced in this PR does check for existing bindings for |
| let existing_binding_for_new_name = semantic.simulate_runtime_load_at_location_in_scope( | ||
| new_name, | ||
| binding.range, | ||
| binding.scope, | ||
| TypingOnlyBindingsStatus::Disallowed, | ||
| ); | ||
|
|
||
| if let Some(binding) = existing_binding_for_new_name.map(|id| semantic.binding(id)) { | ||
| if binding.kind.is_type_param() { | ||
| return Ok(None); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think checking this again is necessary; no tests fail if I remove this added code. We already check this on line 133 with the ShadowedKind logic.
| let existing_binding_for_new_name = semantic.simulate_runtime_load_at_location_in_scope( | |
| new_name, | |
| binding.range, | |
| binding.scope, | |
| TypingOnlyBindingsStatus::Disallowed, | |
| ); | |
| if let Some(binding) = existing_binding_for_new_name.map(|id| semantic.binding(id)) { | |
| if binding.kind.is_type_param() { | |
| return Ok(None); | |
| } | |
| } |
| // Copied from PYI019 | ||
| let reference_is_complex = |id: ResolvedReferenceId| -> bool { | ||
| let reference = semantic.reference(id); | ||
| let in_source = &source[reference.range()]; | ||
|
|
||
| in_source != old_name | ||
| }; |
There was a problem hiding this comment.
I was discussing this issue with @ntBre on Friday -- we think the issue with replacing "complex" stringized references is probably a result of us setting an incorrect range somewhere for ResolvedReferences where the reference is a "complex" stringized reference. I'm okay with this for now, but we should try to see if we can fix the underlying bug rather than working around it repeatedly
There was a problem hiding this comment.
Aha -- it looks like there's already an issue open for the problem about ranges being incorrect for parsed nodes inside complex string annotations. It's here: #10586
AlexWaygood
added
bug
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
Thanks! I pushed a few changes so that this is fixed more holistically, and added some test cases for RUF052 as well.
|
This doesn't close #16024, however. Even on this branch, we get this behaviour: So the fix can still introduce invalid syntax. |
AlexWaygood
changed the title
pyupgrade] Better fix logic (UP049)pyupgrade] [ruff] Don't apply renamings if the new name is shadowed in a scope of one of the references to the binding (UP049, RUF052))
AlexWaygood
changed the title
pyupgrade] [ruff] Don't apply renamings if the new name is shadowed in a scope of one of the references to the binding (UP049, RUF052))pyupgrade] [ruff] Don't apply renamings if the new name is shadowed in a scope of one of the references to the binding (UP049, RUF052)
2 participants
Summary
Helps with #16024.
A fix will no longer be offered if the new name is invalid as an identifier or if it shadows a type parameter from an outer scope, while complex stringified type hints will cause it to be marked as unsafe.
The error message has also been changed from "Remove the leading underscores" to "Rename type parameter to remove leading underscores", as the former would not be the best suggestion when the new name is not a valid identifier (e.g.,
_0).Test Plan
cargo nextest runandcargo insta test.