SONARPY-4111 Fix S3457 false positive for injected Marimo mo.sql calls (#1073)

Co-authored-by: Vibe Bot <vibe-bot@sonarsource.com>
GitOrigin-RevId: 7bdde59ec9c6b50dfd3d7e32d6e61c1cabd0c302

Author: 2 people

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

@@ -20,6 +20,7 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Locale;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.stream.IntStream;
@@ -34,15 +35,19 @@
 import org.sonar.plugins.python.api.tree.DictionaryLiteral;
 import org.sonar.plugins.python.api.tree.DictionaryLiteralElement;
 import org.sonar.plugins.python.api.tree.Expression;
+import org.sonar.plugins.python.api.tree.FunctionDef;
 import org.sonar.plugins.python.api.tree.KeyValuePair;
 import org.sonar.plugins.python.api.tree.Name;
+import org.sonar.plugins.python.api.tree.Parameter;
 import org.sonar.plugins.python.api.tree.QualifiedExpression;
 import org.sonar.plugins.python.api.tree.RegularArgument;
 import org.sonar.plugins.python.api.tree.StringElement;
 import org.sonar.plugins.python.api.tree.StringLiteral;
 import org.sonar.plugins.python.api.tree.Token;
 import org.sonar.plugins.python.api.tree.Tree;
 import org.sonar.python.checks.utils.Expressions;
+import org.sonar.python.checks.utils.MarimoUtils;
+import org.sonar.python.tree.TreeUtils;
 
 @Rule(key = "S3457")
 public class StringFormatCorrectnessCheck extends AbstractStringFormatCheck {
@@ -126,19 +131,49 @@ private static void checkFStringLiteral(SubscriptionContext ctx) {
   }
 
   private static boolean isMarimoSqlArgument(StringLiteral literal, SubscriptionContext ctx) {
+    return enclosingCallExpression(literal)
+      .filter(callExpression -> MARIMO_SQL_MATCHER.isTrueFor(callExpression.callee(), ctx)
+        || isInjectedMarimoSqlCall(callExpression, literal, ctx))
+      .isPresent();
+  }
+
+  private static Optional<CallExpression> enclosingCallExpression(StringLiteral literal) {
     Tree parent = literal.parent();
     if (!(parent instanceof RegularArgument)) {
-      return false;
+      return Optional.empty();
     }
     Tree argList = parent.parent();
     if (!(argList instanceof ArgList)) {
-      return false;
+      return Optional.empty();
     }
     Tree callParent = argList.parent();
     if (!(callParent instanceof CallExpression callExpression)) {
+      return Optional.empty();
+    }
+    return Optional.of(callExpression);
+  }
+
+  private static boolean isInjectedMarimoSqlCall(CallExpression callExpression, StringLiteral literal, SubscriptionContext ctx) {
+    if (!isMoSqlCall(callExpression) || !MarimoUtils.isTreeInMarimoCell(literal, ctx)) {
+      return false;
+    }
+    return Optional.ofNullable(TreeUtils.firstAncestorOfClass(literal, FunctionDef.class))
+      .map(FunctionDef::parameters)
+      .stream()
+      .flatMap(parameters -> parameters.nonTuple().stream())
+      .map(Parameter::name)
+      .filter(Objects::nonNull)
+      .anyMatch(name -> "mo".equals(name.name()));
+  }
+
+  private static boolean isMoSqlCall(CallExpression callExpression) {
+    if (!(callExpression.callee() instanceof QualifiedExpression qualifiedExpression)) {
+      return false;
+    }
+    if (!"sql".equals(qualifiedExpression.name().name())) {
       return false;
     }
-    return MARIMO_SQL_MATCHER.isTrueFor(callExpression.callee(), ctx);
+    return qualifiedExpression.qualifier() instanceof Name qualifier && "mo".equals(qualifier.name());
   }
 
   private static void checkLoggerLog(SubscriptionContext ctx, CallExpression callExpression) {

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

@@ -16,13 +16,57 @@
  */
 package org.sonar.python.checks;
 
+import java.lang.reflect.Method;
+import java.util.Optional;
 import org.junit.jupiter.api.Test;
+import org.sonar.plugins.python.api.tree.ArgList;
+import org.sonar.plugins.python.api.tree.CallExpression;
+import org.sonar.plugins.python.api.tree.RegularArgument;
+import org.sonar.plugins.python.api.tree.StringLiteral;
+import org.sonar.plugins.python.api.tree.Tree;
 import org.sonar.python.checks.utils.PythonCheckVerifier;
 
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
 class StringFormatCorrectnessCheckTest {
 
   @Test
   void test() {
     PythonCheckVerifier.verify("src/test/resources/checks/stringFormatCorrectness.py", new StringFormatCorrectnessCheck());
   }
+
+  @Test
+  void enclosing_call_expression_returns_empty_when_regular_argument_parent_is_not_arg_list() throws Exception {
+    StringLiteral literal = mock(StringLiteral.class);
+    RegularArgument argument = mock(RegularArgument.class);
+    Tree nonArgListParent = mock(Tree.class);
+
+    when(literal.parent()).thenReturn(argument);
+    when(argument.parent()).thenReturn(nonArgListParent);
+
+    assertThat(invokeEnclosingCallExpression(literal)).isEmpty();
+  }
+
+  @Test
+  void enclosing_call_expression_returns_empty_when_arg_list_parent_is_not_call_expression() throws Exception {
+    StringLiteral literal = mock(StringLiteral.class);
+    RegularArgument argument = mock(RegularArgument.class);
+    ArgList argList = mock(ArgList.class);
+    Tree nonCallParent = mock(Tree.class);
+
+    when(literal.parent()).thenReturn(argument);
+    when(argument.parent()).thenReturn(argList);
+    when(argList.parent()).thenReturn(nonCallParent);
+
+    assertThat(invokeEnclosingCallExpression(literal)).isEmpty();
+  }
+
+  @SuppressWarnings("unchecked")
+  private static Optional<CallExpression> invokeEnclosingCallExpression(StringLiteral literal) throws Exception {
+    Method method = StringFormatCorrectnessCheck.class.getDeclaredMethod("enclosingCallExpression", StringLiteral.class);
+    method.setAccessible(true);
+    return (Optional<CallExpression>) method.invoke(null, literal);
+  }
 }

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

@@ -142,3 +142,62 @@ def marimo_sql():
         engine=engine
     )
 
+
+import marimo
+
+app = marimo.App()
+
+
+@app.cell
+def _():
+    import marimo as mo
+
+    engine = None
+    return engine, mo
+
+
+# Previously flagged FP: marimo injects `mo` into a later cell parameter.
+@app.cell
+def _(engine, mo):
+    mo.sql(
+        f"""
+        SELECT id
+        FROM users
+        """,
+        engine=engine
+    )
+
+
+@app.cell
+def _(engine, mo):
+    mo.query(
+        f"SELECT id FROM users"  # Noncompliant {{Add replacement fields or use a normal string instead of an f-string.}}
+    )
+
+
+@app.cell
+def _(engine, session):
+    session.sql(
+        f"SELECT id FROM users"  # Noncompliant {{Add replacement fields or use a normal string instead of an f-string.}}
+    )
+
+
+def not_marimo_cell(engine, mo):
+    mo.sql(
+        f"SELECT id FROM users"  # Noncompliant {{Add replacement fields or use a normal string instead of an f-string.}}
+    )
+
+
+@app.cell
+def _(engine, mo):
+    alias = mo
+    alias.sql(
+        f"SELECT id FROM users"  # Noncompliant {{Add replacement fields or use a normal string instead of an f-string.}}
+    )
+
+
+@app.cell
+def _(engine, mo):
+    sql(
+        f"SELECT id FROM users"  # Noncompliant {{Add replacement fields or use a normal string instead of an f-string.}}
+    )