SONARPY-3986 Fix S905 false positive for bare expressions as marimo cell output (#1044)

Co-authored-by: Vibe Bot <vibe-bot@sonarsource.com>
Co-authored-by: Sebastian Zumbrunn <sebastian.zumbrunn@sonarsource.com>
GitOrigin-RevId: 5b9a81eca800422b6ad02a846ecbe2b018851fcc

Author: 3 people

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

@@ -47,6 +47,7 @@
 import org.sonar.plugins.python.api.tree.UnaryExpression;
 import org.sonar.plugins.python.api.tree.WithItem;
 import org.sonar.plugins.python.api.tree.WithStatement;
+import org.sonar.python.checks.utils.MarimoUtils;
 import org.sonar.python.tree.TreeUtils;
 
 @Rule(key = "S905")
@@ -136,18 +137,42 @@ private static void checkNode(SubscriptionContext ctx) {
     if (isAnAirflowException((Expression) tree, ctx)) {
       return;
     }
+    if (isAMarimoException((Expression) tree, ctx)) {
+      return;
+    }
     ctx.addIssue(tree, MESSAGE);
   }
 
   private static boolean isAnAirflowException(Expression expression, SubscriptionContext ctx) {
     if (isWithinAirflowContext(expression, ctx)) {
       StatementList statementList = (StatementList) TreeUtils.firstAncestorOfKind(expression, Kind.STATEMENT_LIST);
-      return Optional.ofNullable(statementList).map(StatementList::statements).map(statements -> statements.get(statements.size() - 1))
+      return Optional.ofNullable(statementList)
+        .map(StatementList::statements)
+        .map(statements -> statements.get(statements.size() - 1))
         .filter(lastStatement -> lastStatement.equals(TreeUtils.firstAncestorOfKind(expression, Kind.EXPRESSION_STMT))).isPresent();
     }
     return false;
   }
 
+  private static boolean isAMarimoException(Expression expression, SubscriptionContext ctx) {
+    return MarimoUtils.isTreeInMarimoCell(expression, ctx) && isLastExpressionInFunction(expression);
+  }
+
+  private static boolean isLastExpressionInFunction(Expression expr) {
+    FunctionDef functionDef = TreeUtils.firstAncestorOfClass(expr, FunctionDef.class);
+    if (functionDef == null) {
+      return false;
+    }
+    StatementList statementList = functionDef.body();
+
+    Tree expressionStmt = TreeUtils.firstAncestorOfKind(expr, Kind.EXPRESSION_STMT);
+    return statementList.statements().stream()
+      .filter(s -> s.is(Kind.EXPRESSION_STMT))
+      .reduce((first, second) -> second)
+      .filter(lastExprStmt -> lastExprStmt.equals(expressionStmt))
+      .isPresent();
+  }
+
   private static boolean isWithinIgnoredContext(Tree tree, SubscriptionContext ctx) {
     Tree withParent = TreeUtils.firstAncestorOfKind(tree, Kind.WITH_STMT);
     if (withParent != null) {

python-checks/src/main/java/org/sonar/python/checks/utils/MarimoUtils.java

@@ -43,11 +43,31 @@ public static boolean isTreeInMarimoDecoratedFunction(Tree tree, SubscriptionCon
     return functionDef.decorators().stream().anyMatch(decorator -> isAppCellDecorator(decorator, ctx));
   }
 
+  public static boolean isTreeInMarimoCell(Tree tree, SubscriptionContext ctx) {
+    FunctionDef functionDef = (FunctionDef) TreeUtils.firstAncestorOfKind(tree, Kind.FUNCDEF);
+    if (functionDef == null) {
+      return false;
+    }
+    return functionDef.decorators().stream().anyMatch(d -> isAppCell(d, ctx));
+  }
+
+  private static boolean isAppCell(Decorator decorator, SubscriptionContext ctx) {
+    return APP_CELL_MATCHER.isTrueFor(resolveDecoratorExpression(decorator), ctx);
+  }
+
+  private static boolean isAppFunction(Decorator decorator, SubscriptionContext ctx) {
+    return APP_FUNCTION_MATCHER.isTrueFor(resolveDecoratorExpression(decorator), ctx);
+  }
+
   private static boolean isAppCellDecorator(Decorator decorator, SubscriptionContext ctx) {
+    return isAppCell(decorator, ctx) || isAppFunction(decorator, ctx);
+  }
+
+  private static Expression resolveDecoratorExpression(Decorator decorator) {
     Expression expr = decorator.expression();
     if (expr instanceof CallExpression callExpr) {
-      expr = callExpr.callee();
+      return callExpr.callee();
     }
-    return APP_CELL_MATCHER.isTrueFor(expr, ctx) || APP_FUNCTION_MATCHER.isTrueFor(expr, ctx);
+    return expr;
   }
 }

python-checks/src/test/java/org/sonar/python/checks/UselessStatementCheckTest.java

@@ -47,4 +47,10 @@ void test_import() {
   void test_ignore_manifest() {
     PythonCheckVerifier.verifyNoIssue("src/test/resources/checks/__manifest__.py", new UselessStatementCheck());
   }
+
+  @Test
+  void test_marimo() {
+    PythonCheckVerifier.verify("src/test/resources/checks/uselessStatement/uselessStatementMarimo.py", new UselessStatementCheck());
+  }
+
 }

python-checks/src/test/resources/checks/uselessStatement/uselessStatementMarimo.py

@@ -0,0 +1,65 @@
+import marimo
+
+app = marimo.App()
+
+
+@app.cell
+def _(combined_df):
+    combined_df
+    return
+
+
+@app.cell
+def _(_fig):
+    _fig
+    return
+
+
+@app.cell(hide_code=True)
+def _(result_df):
+    result_df
+    return
+
+
+@app.cell
+def _(df):
+    df
+
+
+@app.cell
+def _(x, y):
+    x  # Noncompliant
+    y
+    return
+
+
+@app.cell
+def _(df):
+    df
+    if True:  # control flow after cell output
+        pass
+
+
+@app.cell
+def _(x, y):
+    x  # Noncompliant
+    if True:  # control flow after non-last expression
+        pass
+    y
+
+
+@app.cell
+def _(x):
+    if True:
+        x  # Noncompliant
+
+
+@app.function
+def helper(x):
+    x  # Noncompliant
+    return x
+
+
+def not_marimo(x):
+    x  # Noncompliant
+    return x