Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[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

Merged
merged 6 commits into from
Feb 8, 2025

Conversation

InSyncWithFoo
Copy link
Contributor

@InSyncWithFoo InSyncWithFoo commented Feb 8, 2025

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 run and cargo insta test.

Copy link
Contributor

github-actions bot commented Feb 8, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@InSyncWithFoo
Copy link
Contributor Author

@ntBre Renamer has a small problem in that it would also rename _T in this case:

class C[_T]:
	def f[T](self):
		a: _T = ...

The new logic introduced in this PR does check for existing bindings for T, but only at the top of the class, so it won't be able to detect bindings declared within inner scopes. This is a general problem with Renamer, so I decided not to fix it for now; instead, I added a few test cases.

Comment on lines 152 to 164
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);
}
}

Copy link
Member

@AlexWaygood AlexWaygood Feb 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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);
}
}

Comment on lines 140 to 146
// 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
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@AlexWaygood AlexWaygood Feb 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 AlexWaygood added bug Something isn't working fixes Related to suggested fixes for violations labels Feb 8, 2025
.
.
.
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I pushed a few changes so that this is fixed more holistically, and added some test cases for RUF052 as well.

@AlexWaygood
Copy link
Member

This doesn't close #16024, however. Even on this branch, we get this behaviour:

~/dev/ruff (UP049)⚡ [1] % cargo run -- check --no-cache --preview --diff --select=UP049 --target-version=py312 foo.py
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.12s
     Running `target/debug/ruff check --no-cache --preview --diff --select=UP049 --target-version=py312 foo.py`
--- foo.py
+++ foo.py
@@ -1 +1 @@
-class Foo[_T, __T]: ...
+class Foo[T, T]: ...

Would fix 2 errors.

So the fix can still introduce invalid syntax.

@AlexWaygood 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)) Feb 8, 2025
@AlexWaygood 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) Feb 8, 2025
@AlexWaygood AlexWaygood merged commit a04ddf2 into astral-sh:main Feb 8, 2025
21 checks passed
@InSyncWithFoo InSyncWithFoo deleted the UP049 branch February 8, 2025 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixes Related to suggested fixes for violations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants