Skip to content

[ty] Add subdiagnostic suggestion to unresolved-reference diagnostic when variable exists on self#18444

Merged
carljm merged 6 commits intoastral-sh:mainfrom
lipefree:suggest-self
Jun 4, 2025
Merged

[ty] Add subdiagnostic suggestion to unresolved-reference diagnostic when variable exists on self#18444
carljm merged 6 commits intoastral-sh:mainfrom
lipefree:suggest-self

Conversation

@lipefree
Copy link
Contributor

@lipefree lipefree commented Jun 3, 2025

Summary

Implements astral-sh/ty#502.

In the following example:

class Foo:
    x: int

    def method(self):
        y = x

The user may intended to use y = self.x in method.

This is now added as a subdiagnostic in the following form :

info: An attribute with the same name as 'x' is defined, consider using 'self.x'

Currently the following will not raise the subdiagnostic which is what I am working on to close this PR:

class Foo:
    x: int = 0

    def method(self):
        y = x

Test Plan

I will add mdtests with snapshots to capture the subdiagnostic (I just have to find the right file).

@github-actions
Copy link
Contributor

github-actions bot commented Jun 3, 2025

mypy_primer results

No ecosystem changes detected ✅

@ntBre ntBre added the ty Multi-file analysis & type inference label Jun 3, 2025
@AlexWaygood AlexWaygood added the diagnostics Related to reporting of diagnostics. label Jun 4, 2025
@lipefree lipefree marked this pull request as ready for review June 4, 2025 14:00
@lipefree lipefree marked this pull request as draft June 4, 2025 14:11
@lipefree
Copy link
Contributor Author

lipefree commented Jun 4, 2025

Sorry I did not see the panic found by the fuzzing, I will work on it

@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 4, 2025

if you make this change to the TypeInferenceBuilder::class_context_of_current_method() method on your PR branch, it will fix the fuzzing failure:

diff --git a/crates/ty_python_semantic/src/types/infer.rs b/crates/ty_python_semantic/src/types/infer.rs
index abb6f91206..8b627ac33c 100644
--- a/crates/ty_python_semantic/src/types/infer.rs
+++ b/crates/ty_python_semantic/src/types/infer.rs
@@ -1714,6 +1714,10 @@ impl<'db> TypeInferenceBuilder<'db> {
     fn class_context_of_current_method(&self) -> Option<ClassLiteral<'db>> {
         let current_scope_id = self.scope().file_scope_id(self.db());
         let current_scope = self.index.scope(current_scope_id);
+        if current_scope.kind() != ScopeKind::Function {
+            return None;
+        }
+
         let parent_scope_id = current_scope.parent()?;
         let parent_scope = self.index.scope(parent_scope_id);

It's a pre-existing bug that wasn't caused by your PR -- it's just been exposed by your PR!

@lipefree
Copy link
Contributor Author

lipefree commented Jun 4, 2025

if you make this change to your PR branch, it will fix the fuzzing failure:

diff --git a/crates/ty_python_semantic/src/types/infer.rs b/crates/ty_python_semantic/src/types/infer.rs
index abb6f91206..8b627ac33c 100644
--- a/crates/ty_python_semantic/src/types/infer.rs
+++ b/crates/ty_python_semantic/src/types/infer.rs
@@ -1714,6 +1714,10 @@ impl<'db> TypeInferenceBuilder<'db> {
     fn class_context_of_current_method(&self) -> Option<ClassLiteral<'db>> {
         let current_scope_id = self.scope().file_scope_id(self.db());
         let current_scope = self.index.scope(current_scope_id);
+        if current_scope.kind() != ScopeKind::Function {
+            return None;
+        }
+
         let parent_scope_id = current_scope.parent()?;
         let parent_scope = self.index.scope(parent_scope_id);

It's a pre-existing bug that wasn't caused by your PR -- it's just been exposed by your PR!

Thank you so much @AlexWaygood, I was going for a full investigation but on my code only ! You are saving me so much time here ! I will then add it to my PR!

@lipefree lipefree marked this pull request as ready for review June 4, 2025 14:46
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Looks great, thank you! Couple of minor nits.

@carljm carljm merged commit e658778 into astral-sh:main Jun 4, 2025
34 checks passed
AlexWaygood added a commit that referenced this pull request Jun 4, 2025
…c if we know we'll emit the diagnostic (#18465)

## Summary

This optimizes some of the logic added in
#18444. In general, we only
calculate information for subdiagnostics if we know we'll actually emit
the diagnostic. The check to see whether we'll emit the diagnostic is
work we'll definitely have to do whereas the the work to gather
information for a subdiagnostic isn't work we necessarily have to do if
the diagnostic isn't going to be emitted at all.

This PR makes us lazier about gathering the information we need for the
subdiagnostic, and moves all the subdiagnostic logic into one function
rather than having some `unresolved-reference` subdiagnostic logic in
`infer.rs` and some in `diagnostic.rs`.

## Test Plan

`cargo test -p ty_python_semantic`
carljm added a commit to mtshiba/ruff that referenced this pull request Jun 4, 2025
* main:
  [ty] Only consider a type `T` a subtype of a protocol `P` if all of `P`'s members are fully bound on `T` (astral-sh#18466)
  [ty] Exclude members starting with `_abc_` from a protocol interface (astral-sh#18467)
  [ty] Add subdiagnostic suggestion to `unresolved-reference` diagnostic when variable exists on `self` (astral-sh#18444)
  [ty] IDE: only provide declarations and bindings as completions (astral-sh#18456)
  [ty] ty_ide: Hotfix for `expression_scope_id` panics (astral-sh#18455)
  [ty] Improve docs for Class{Literal,Type}::instance_member (astral-sh#18454)
  [ty] Add meta-type tests for legavy TypeVars (astral-sh#18453)
  update to salsa that doesn't panic silently on cycles (astral-sh#18450)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

diagnostics Related to reporting of diagnostics. ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants