SONARPY-2939 Fix FP on S7503 when implementing protocol methods (#290)

GitOrigin-RevId: dca9fffc4f550a473a00b0718a72434775b22063

Author: guillaume-dequenne

python-checks/src/main/java/org/sonar/python/checks/AsyncFunctionNotAsyncCheck.java

@@ -31,37 +31,48 @@
 import org.sonar.plugins.python.api.tree.WithStatement;
 import org.sonar.plugins.python.api.tree.YieldExpression;
 import org.sonar.plugins.python.api.tree.YieldStatement;
+import org.sonar.plugins.python.api.types.v2.ClassType;
+import org.sonar.plugins.python.api.types.v2.FunctionType;
 import org.sonar.python.checks.utils.CheckUtils;
 
 @Rule(key = "S7503")
 public class AsyncFunctionNotAsyncCheck extends PythonSubscriptionCheck {
-  
+
   private static final String MESSAGE = "Use asynchronous features in this function or remove the `async` keyword.";
-  
+
   @Override
   public void initialize(Context context) {
     context.registerSyntaxNodeConsumer(Tree.Kind.FUNCDEF, AsyncFunctionNotAsyncCheck::checkAsyncFunction);
   }
-  
+
   private static void checkAsyncFunction(SubscriptionContext ctx) {
     FunctionDef functionDef = (FunctionDef) ctx.syntaxNode();
-    
+
     Token asyncKeyword = functionDef.asyncKeyword();
     if (asyncKeyword == null || isException(functionDef)) {
       return;
     }
     AsyncFeatureVisitor visitor = new AsyncFeatureVisitor();
     functionDef.body().accept(visitor);
-    
+
     if (!visitor.hasAsyncFeature()) {
       ctx.addIssue(functionDef.name(), MESSAGE).secondary(asyncKeyword, "This function is async.");
     }
   }
 
   private static boolean isException(FunctionDef functionDef) {
-    return CheckUtils.isAbstract(functionDef) || isEmptyFunction(functionDef.body());
+    return CheckUtils.isAbstract(functionDef) ||
+      isEmptyFunction(functionDef.body()) ||
+      isDunderMethod(functionDef) ||
+      !functionDef.decorators().isEmpty() ||
+      mightBeOverridingMethod(functionDef);
   }
-  
+
+  private static boolean isDunderMethod(FunctionDef functionDef) {
+    String methodName = functionDef.name().name();
+    return methodName.startsWith("__");
+  }
+
   private static boolean isEmptyFunction(StatementList body) {
     for (Statement statement : body.statements()) {
       if (!CheckUtils.isEmptyStatement(statement)) {
@@ -70,20 +81,25 @@ private static boolean isEmptyFunction(StatementList body) {
     }
     return true;
   }
-  
+
+  private static boolean mightBeOverridingMethod(FunctionDef functionDef) {
+    FunctionType functionType = (FunctionType) functionDef.name().typeV2();
+    return functionType.owner() instanceof ClassType classType && (classType.hasUnresolvedHierarchy() || classType.inheritedMember(functionType.name()).isPresent());
+  }
+
   private static class AsyncFeatureVisitor extends BaseTreeVisitor {
-    
+
     private boolean asyncFeatureFound = false;
-    
+
     public boolean hasAsyncFeature() {
       return asyncFeatureFound;
     }
-    
+
     @Override
     public void visitAwaitExpression(AwaitExpression awaitExpression) {
       asyncFeatureFound = true;
     }
-    
+
     @Override
     public void visitForStatement(ForStatement forStatement) {
       if (forStatement.isAsync()) {
@@ -94,7 +110,7 @@ public void visitForStatement(ForStatement forStatement) {
         super.visitForStatement(forStatement);
       }
     }
-    
+
     @Override
     public void visitWithStatement(WithStatement withStatement) {
       if (withStatement.isAsync()) {
@@ -104,22 +120,22 @@ public void visitWithStatement(WithStatement withStatement) {
         super.visitWithStatement(withStatement);
       }
     }
-    
+
     @Override
     public void visitYieldStatement(YieldStatement yieldStatement) {
       asyncFeatureFound = true;
     }
-    
+
     @Override
     public void visitYieldExpression(YieldExpression yieldExpression) {
       asyncFeatureFound = true;
     }
-    
+
     @Override
     public void visitFunctionDef(FunctionDef functionDef) {
       // Skip nested functions
     }
-    
+
     @Override
     protected void scan(@Nullable Tree tree) {
       // Stop scanning if we've already found an async feature

python-checks/src/test/resources/checks/asyncFunctionNotAsync.py

@@ -84,7 +84,7 @@ async def async_method_with_await(self):  # Compliant
         return await self.some_coroutine()
 
     @classmethod
-    async def async_classmethod_without_await(cls):  # Noncompliant
+    async def async_classmethod_without_await(cls):  # Avoid FPs with decorators
         return cls.some_value
 
     async def async_method_with_inner_function(self):
@@ -101,9 +101,75 @@ async def abstract_async_method_2(self):  # Compliant
         raise NotImplementedError("This is an abstract method")
 
     @abc.other
-    async def other_decorator_1(self):  # Noncompliant
+    async def other_decorator_1(self):  # Avoid FPs with decorators
         raise NotImplementedError("...")
 
     @unknown()
-    async def other_decorator_1(self):  # Noncompliant
+    async def other_decorator_1(self):  # Avoid FPs with decorators
         raise NotImplementedError("...")
+
+# Async protocol methods - should be compliant even without await
+class AsyncContextManager:
+    async def __aenter__(self):
+        return self
+
+    async def __aexit__(self, exc_type, exc_val, exc_tb):
+        pass
+
+    async def __unknown_dunder__(self):
+        # Avoid risk of FPs
+        pass
+
+class AsyncIterator:
+    async def __aiter__(self):
+        return self
+
+    async def __anext__(self):
+        if self.should_stop():
+            raise StopAsyncIteration
+        return self.value
+
+class AsyncResource:
+    async def __aclose__(self):
+        print("Releasing resources")
+        
+class AsyncAwaitableObject:
+    async def __await__(self):
+        yield "something"
+
+    async def regular_method_without_await(self):  # Noncompliant
+        print("This is not a protocol method")
+
+# FastAPI route examples
+from fastapi import FastAPI, APIRouter
+
+app = FastAPI()
+router = APIRouter()
+
+@app.get("/items/{item_id}")
+async def read_item(item_id: int):  # Compliant - FastAPI route
+    return {"item_id": item_id}
+
+@app.post("/users/")
+async def create_user(user_data: dict):  # Compliant - FastAPI route
+    # No await, but this is still valid for FastAPI routes
+    return {"user_id": 123, "data": user_data}
+
+@router.put("/items/{item_id}")
+async def update_item(item_id: int, item: dict):  # Compliant - FastAPI route via router
+    return {"item_id": item_id, "item": item}
+
+@app.delete("/items/{item_id}")
+async def delete_item(item_id: int):  # Compliant - FastAPI route
+    # No await, but this is still valid for FastAPI routes
+    return {"deleted": True}
+
+
+class MyClass:
+    async def my_method(self):
+        await something()
+
+class MyOtherClass(MyClass):
+    async def my_method(self):
+        # No issue on overriding methods
+        do_something()