[ty] Don't introduce invalid syntax when autofixing override-of-final-method (#21699)

Author: AlexWaygood

crates/ruff_db/src/diagnostic/mod.rs

@@ -354,6 +354,13 @@ impl Diagnostic {
         Arc::make_mut(&mut self.inner).fix = Some(fix);
     }
 
+    /// If `fix` is `Some`, set the fix for this diagnostic.
+    pub fn set_optional_fix(&mut self, fix: Option<Fix>) {
+        if let Some(fix) = fix {
+            self.set_fix(fix);
+        }
+    }
+
     /// Remove the fix for this diagnostic.
     pub fn remove_fix(&mut self) {
         Arc::make_mut(&mut self.inner).fix = None;

crates/ty_python_semantic/resources/mdtest/final.md

@@ -51,6 +51,10 @@ class Parent:
     @final
     def my_property2(self) -> int: ...
 
+    @property
+    @final
+    def my_property3(self) -> int: ...
+
     @final
     @classmethod
     def class_method1(cls) -> int: ...
@@ -86,6 +90,13 @@ class Child(Parent):
 
     @property
     def my_property2(self) -> int: ...  # error: [override-of-final-method]
+    @my_property2.setter
+    def my_property2(self, x: int) -> None: ...
+
+    @property
+    def my_property3(self) -> int: ...  # error: [override-of-final-method]
+    @my_property3.deleter
+    def my_proeprty3(self) -> None: ...
 
     @classmethod
     def class_method1(cls) -> int: ...  # error: [override-of-final-method]
@@ -230,7 +241,7 @@ class ChildOfBad(Bad):
     def bar(self, x: str) -> str: ...
     @overload
     def bar(self, x: int) -> int: ...  # error: [override-of-final-method]
-    
+
     @overload
     def baz(self, x: str) -> str: ...
     @overload
@@ -461,14 +472,17 @@ class B(A):
     def method1(self) -> None: ...  # error: [override-of-final-method]
     def method2(self) -> None: ...  # error: [override-of-final-method]
     def method3(self) -> None: ...  # error: [override-of-final-method]
-    def method4(self) -> None: ...  # error: [override-of-final-method]
+
+    # check that autofixes don't introduce invalid syntax
+    # if there are multiple statements on one line
+    #
+    # TODO: we should emit a Liskov violation here too
+    # error: [override-of-final-method]
+    method4 = 42; unrelated = 56  # fmt: skip
 
 # Possible overrides of possibly `@final` methods...
 class C(A):
     if coinflip():
-        # TODO: the autofix here introduces invalid syntax because there are now no
-        # statements inside the `if:` branch
-        # (but it might still be a useful autofix in an IDE context?)
         def method1(self) -> None: ...  # error: [override-of-final-method]
     else:
         pass

crates/ty_python_semantic/resources/mdtest/snapshots/final.md_-_Tests_for_the_`@typi…_-_A_possibly-undefined…_(fc7b496fd1986deb).snap

@@ -49,26 +49,29 @@ mdtest path: crates/ty_python_semantic/resources/mdtest/final.md
 35 |     def method1(self) -> None: ...  # error: [override-of-final-method]
 36 |     def method2(self) -> None: ...  # error: [override-of-final-method]
 37 |     def method3(self) -> None: ...  # error: [override-of-final-method]
-38 |     def method4(self) -> None: ...  # error: [override-of-final-method]
-39 | 
-40 | # Possible overrides of possibly `@final` methods...
-41 | class C(A):
-42 |     if coinflip():
-43 |         # TODO: the autofix here introduces invalid syntax because there are now no
-44 |         # statements inside the `if:` branch
-45 |         # (but it might still be a useful autofix in an IDE context?)
-46 |         def method1(self) -> None: ...  # error: [override-of-final-method]
-47 |     else:
-48 |         pass
-49 | 
-50 |     if coinflip():
-51 |         def method2(self) -> None: ...  # TODO: should emit [override-of-final-method]
-52 |     else:
-53 |         def method2(self) -> None: ...  # TODO: should emit [override-of-final-method]
-54 | 
-55 |     if coinflip():
-56 |         def method3(self) -> None: ...  # error: [override-of-final-method]
-57 |         def method4(self) -> None: ...  # error: [override-of-final-method]
+38 | 
+39 |     # check that autofixes don't introduce invalid syntax
+40 |     # if there are multiple statements on one line
+41 |     #
+42 |     # TODO: we should emit a Liskov violation here too
+43 |     # error: [override-of-final-method]
+44 |     method4 = 42; unrelated = 56  # fmt: skip
+45 | 
+46 | # Possible overrides of possibly `@final` methods...
+47 | class C(A):
+48 |     if coinflip():
+49 |         def method1(self) -> None: ...  # error: [override-of-final-method]
+50 |     else:
+51 |         pass
+52 | 
+53 |     if coinflip():
+54 |         def method2(self) -> None: ...  # TODO: should emit [override-of-final-method]
+55 |     else:
+56 |         def method2(self) -> None: ...  # TODO: should emit [override-of-final-method]
+57 | 
+58 |     if coinflip():
+59 |         def method3(self) -> None: ...  # error: [override-of-final-method]
+60 |         def method4(self) -> None: ...  # error: [override-of-final-method]
 ```
 
 # Diagnostics
@@ -104,7 +107,7 @@ info: rule `override-of-final-method` is enabled by default
 35 +       # error: [override-of-final-method]
 36 |     def method2(self) -> None: ...  # error: [override-of-final-method]
 37 |     def method3(self) -> None: ...  # error: [override-of-final-method]
-38 |     def method4(self) -> None: ...  # error: [override-of-final-method]
+38 | 
 note: This is an unsafe fix and may change runtime behavior
 
 ```
@@ -118,7 +121,6 @@ error[override-of-final-method]: Cannot override `A.method2`
 36 |     def method2(self) -> None: ...  # error: [override-of-final-method]
    |         ^^^^^^^ Overrides a definition from superclass `A`
 37 |     def method3(self) -> None: ...  # error: [override-of-final-method]
-38 |     def method4(self) -> None: ...  # error: [override-of-final-method]
    |
 info: `A.method2` is decorated with `@final`, forbidding overrides
   --> src/mdtest_snippet.py:16:9
@@ -140,8 +142,8 @@ info: rule `override-of-final-method` is enabled by default
    -     def method2(self) -> None: ...  # error: [override-of-final-method]
 36 +       # error: [override-of-final-method]
 37 |     def method3(self) -> None: ...  # error: [override-of-final-method]
-38 |     def method4(self) -> None: ...  # error: [override-of-final-method]
-39 | 
+38 | 
+39 |     # check that autofixes don't introduce invalid syntax
 note: This is an unsafe fix and may change runtime behavior
 
 ```
@@ -154,7 +156,8 @@ error[override-of-final-method]: Cannot override `A.method3`
 36 |     def method2(self) -> None: ...  # error: [override-of-final-method]
 37 |     def method3(self) -> None: ...  # error: [override-of-final-method]
    |         ^^^^^^^ Overrides a definition from superclass `A`
-38 |     def method4(self) -> None: ...  # error: [override-of-final-method]
+38 |
+39 |     # check that autofixes don't introduce invalid syntax
    |
 info: `A.method3` is decorated with `@final`, forbidding overrides
   --> src/mdtest_snippet.py:20:9
@@ -174,23 +177,23 @@ info: rule `override-of-final-method` is enabled by default
 36 |     def method2(self) -> None: ...  # error: [override-of-final-method]
    -     def method3(self) -> None: ...  # error: [override-of-final-method]
 37 +       # error: [override-of-final-method]
-38 |     def method4(self) -> None: ...  # error: [override-of-final-method]
-39 | 
-40 | # Possible overrides of possibly `@final` methods...
+38 | 
+39 |     # check that autofixes don't introduce invalid syntax
+40 |     # if there are multiple statements on one line
 note: This is an unsafe fix and may change runtime behavior
 
 ```
 
 ```
 error[override-of-final-method]: Cannot override `A.method4`
-  --> src/mdtest_snippet.py:38:9
+  --> src/mdtest_snippet.py:44:5
    |
-36 |     def method2(self) -> None: ...  # error: [override-of-final-method]
-37 |     def method3(self) -> None: ...  # error: [override-of-final-method]
-38 |     def method4(self) -> None: ...  # error: [override-of-final-method]
-   |         ^^^^^^^ Overrides a definition from superclass `A`
-39 |
-40 | # Possible overrides of possibly `@final` methods...
+42 |     # TODO: we should emit a Liskov violation here too
+43 |     # error: [override-of-final-method]
+44 |     method4 = 42; unrelated = 56  # fmt: skip
+   |     ^^^^^^^ Overrides a definition from superclass `A`
+45 |
+46 | # Possible overrides of possibly `@final` methods...
    |
 info: `A.method4` is decorated with `@final`, forbidding overrides
   --> src/mdtest_snippet.py:29:9
@@ -206,28 +209,19 @@ info: `A.method4` is decorated with `@final`, forbidding overrides
    |
 help: Remove the override of `method4`
 info: rule `override-of-final-method` is enabled by default
-35 |     def method1(self) -> None: ...  # error: [override-of-final-method]
-36 |     def method2(self) -> None: ...  # error: [override-of-final-method]
-37 |     def method3(self) -> None: ...  # error: [override-of-final-method]
-   -     def method4(self) -> None: ...  # error: [override-of-final-method]
-38 +       # error: [override-of-final-method]
-39 | 
-40 | # Possible overrides of possibly `@final` methods...
-41 | class C(A):
-note: This is an unsafe fix and may change runtime behavior
 
 ```
 
 ```
 error[override-of-final-method]: Cannot override `A.method1`
-  --> src/mdtest_snippet.py:46:13
+  --> src/mdtest_snippet.py:49:13
    |
-44 |         # statements inside the `if:` branch
-45 |         # (but it might still be a useful autofix in an IDE context?)
-46 |         def method1(self) -> None: ...  # error: [override-of-final-method]
+47 | class C(A):
+48 |     if coinflip():
+49 |         def method1(self) -> None: ...  # error: [override-of-final-method]
    |             ^^^^^^^ Overrides a definition from superclass `A`
-47 |     else:
-48 |         pass
+50 |     else:
+51 |         pass
    |
 info: `A.method1` is decorated with `@final`, forbidding overrides
   --> src/mdtest_snippet.py:8:9
@@ -243,26 +237,17 @@ info: `A.method1` is decorated with `@final`, forbidding overrides
    |
 help: Remove the override of `method1`
 info: rule `override-of-final-method` is enabled by default
-43 |         # TODO: the autofix here introduces invalid syntax because there are now no
-44 |         # statements inside the `if:` branch
-45 |         # (but it might still be a useful autofix in an IDE context?)
-   -         def method1(self) -> None: ...  # error: [override-of-final-method]
-46 +           # error: [override-of-final-method]
-47 |     else:
-48 |         pass
-49 | 
-note: This is an unsafe fix and may change runtime behavior
 
 ```
 
 ```
 error[override-of-final-method]: Cannot override `A.method3`
-  --> src/mdtest_snippet.py:56:13
+  --> src/mdtest_snippet.py:59:13
    |
-55 |     if coinflip():
-56 |         def method3(self) -> None: ...  # error: [override-of-final-method]
+58 |     if coinflip():
+59 |         def method3(self) -> None: ...  # error: [override-of-final-method]
    |             ^^^^^^^ Overrides a definition from superclass `A`
-57 |         def method4(self) -> None: ...  # error: [override-of-final-method]
+60 |         def method4(self) -> None: ...  # error: [override-of-final-method]
    |
 info: `A.method3` is decorated with `@final`, forbidding overrides
   --> src/mdtest_snippet.py:20:9
@@ -277,23 +262,16 @@ info: `A.method3` is decorated with `@final`, forbidding overrides
    |
 help: Remove the override of `method3`
 info: rule `override-of-final-method` is enabled by default
-53 |         def method2(self) -> None: ...  # TODO: should emit [override-of-final-method]
-54 | 
-55 |     if coinflip():
-   -         def method3(self) -> None: ...  # error: [override-of-final-method]
-56 +           # error: [override-of-final-method]
-57 |         def method4(self) -> None: ...  # error: [override-of-final-method]
-note: This is an unsafe fix and may change runtime behavior
 
 ```
 
 ```
 error[override-of-final-method]: Cannot override `A.method4`
-  --> src/mdtest_snippet.py:57:13
+  --> src/mdtest_snippet.py:60:13
    |
-55 |     if coinflip():
-56 |         def method3(self) -> None: ...  # error: [override-of-final-method]
-57 |         def method4(self) -> None: ...  # error: [override-of-final-method]
+58 |     if coinflip():
+59 |         def method3(self) -> None: ...  # error: [override-of-final-method]
+60 |         def method4(self) -> None: ...  # error: [override-of-final-method]
    |             ^^^^^^^ Overrides a definition from superclass `A`
    |
 info: `A.method4` is decorated with `@final`, forbidding overrides
@@ -310,11 +288,5 @@ info: `A.method4` is decorated with `@final`, forbidding overrides
    |
 help: Remove the override of `method4`
 info: rule `override-of-final-method` is enabled by default
-54 | 
-55 |     if coinflip():
-56 |         def method3(self) -> None: ...  # error: [override-of-final-method]
-   -         def method4(self) -> None: ...  # error: [override-of-final-method]
-57 +           # error: [override-of-final-method]
-note: This is an unsafe fix and may change runtime behavior
 
 ```