SONARPY-4108 Implement S8503: Membership tests should not use empty collections (#1070)

GitOrigin-RevId: f752954d92186ed151b82c3c52f91df153b06786

Author: guillaume-dequenne

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

@@ -0,0 +1,72 @@
+/*
+ * SonarQube Python Plugin
+ * Copyright (C) SonarSource Sàrl
+ * mailto:info AT sonarsource DOT com
+ *
+ * You can redistribute and/or modify this program under the terms of
+ * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ * See the Sonar Source-Available License for more details.
+ *
+ * You should have received a copy of the Sonar Source-Available License
+ * along with this program; if not, see https://sonarsource.com/license/ssal/
+ */
+package org.sonar.python.checks;
+
+import org.sonar.check.Rule;
+import org.sonar.plugins.python.api.PythonSubscriptionCheck;
+import org.sonar.plugins.python.api.SubscriptionContext;
+import org.sonar.plugins.python.api.tree.CallExpression;
+import org.sonar.plugins.python.api.tree.DictionaryLiteral;
+import org.sonar.plugins.python.api.tree.Expression;
+import org.sonar.plugins.python.api.tree.InExpression;
+import org.sonar.plugins.python.api.tree.ListLiteral;
+import org.sonar.plugins.python.api.tree.Tree;
+import org.sonar.plugins.python.api.tree.Tuple;
+import org.sonar.plugins.python.api.types.v2.matchers.TypeMatcher;
+import org.sonar.plugins.python.api.types.v2.matchers.TypeMatchers;
+
+@Rule(key = "S8503")
+public class EmptyCollectionMembershipTestCheck extends PythonSubscriptionCheck {
+
+  private static final String MESSAGE = "Remove this membership test on an empty collection; it will always be the same value.";
+
+  private static final TypeMatcher EMPTY_COLLECTION_CONSTRUCTOR = TypeMatchers.any(
+    TypeMatchers.isType("builtins.set"),
+    TypeMatchers.isType("builtins.tuple"),
+    TypeMatchers.isType("builtins.frozenset")
+  );
+
+  @Override
+  public void initialize(Context context) {
+    context.registerSyntaxNodeConsumer(Tree.Kind.IN, EmptyCollectionMembershipTestCheck::checkInExpression);
+  }
+
+  private static void checkInExpression(SubscriptionContext ctx) {
+    InExpression inExpression = (InExpression) ctx.syntaxNode();
+    Expression rightOperand = inExpression.rightOperand();
+    if (isEmptyCollection(rightOperand, ctx)) {
+      ctx.addIssue(inExpression, MESSAGE);
+    }
+  }
+
+  private static boolean isEmptyCollection(Expression expression, SubscriptionContext ctx) {
+    if (expression instanceof ListLiteral listLiteral) {
+      return listLiteral.elements().expressions().isEmpty();
+    }
+    if (expression instanceof DictionaryLiteral dictionaryLiteral) {
+      return dictionaryLiteral.elements().isEmpty();
+    }
+    if (expression instanceof Tuple tuple) {
+      return tuple.elements().isEmpty();
+    }
+    if (expression instanceof CallExpression callExpression) {
+      return callExpression.arguments().isEmpty()
+        && EMPTY_COLLECTION_CONSTRUCTOR.isTrueFor(callExpression.callee(), ctx);
+    }
+    return false;
+  }
+}

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

@@ -213,6 +213,7 @@ public Stream<Class<?>> getChecks() {
       EmailSendingCheck.class,
       EmptyAlternativeCheck.class,
       EmptyCollectionConstructorCheck.class,
+      EmptyCollectionMembershipTestCheck.class,
       EmptyGroupCheck.class,
       EmptyFunctionCheck.class,
       EmptyNestedBlockCheck.class,

python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S8503.html

@@ -0,0 +1,65 @@
+<p>This rule raises an issue when a membership test using <code>in</code> or <code>not in</code> is performed on an empty collection literal such as
+<code>[]</code>, <code>{}</code>, <code>set()</code>, <code>tuple()</code>, or <code>frozenset()</code>.</p>
+<h2>Why is this an issue?</h2>
+<p>In Python, the membership operators <code>in</code> and <code>not in</code> test whether a value exists in a collection. When the collection is
+empty, these tests always produce predictable results:</p>
+<ul>
+  <li><code>x in []</code> is always <code>False</code> because the value cannot exist in an empty collection</li>
+  <li><code>x not in []</code> is always <code>True</code> because the value is indeed not in an empty collection</li>
+</ul>
+<p>This pattern typically indicates one of three problems:</p>
+<ul>
+  <li><strong>dead code</strong>: the check serves no purpose and should be removed</li>
+  <li><strong>logic error</strong>: the collection should have been populated before the membership test</li>
+  <li><strong>incomplete implementation</strong>: the code was started but never finished</li>
+</ul>
+<p>Consider this example:</p>
+<pre>
+if user_id in []:
+    grant_access()
+</pre>
+<p>This condition will never be true, so <code>grant_access()</code> will never be called. The code is either incomplete, meaning the list should
+contain authorized user IDs, or the check is unnecessary and should be removed.</p>
+<p>The issue applies to all Python collection types:</p>
+<ul>
+  <li>empty list: <code>[]</code></li>
+  <li>empty dict: <code>{}</code>, which creates an empty dictionary, not a set. When used with <code>in</code>, it tests keys.</li>
+  <li>empty set: <code>set()</code></li>
+  <li>empty tuple: <code>()</code> or <code>tuple()</code></li>
+  <li>empty frozenset: <code>frozenset()</code></li>
+</ul>
+<h3>What is the potential impact?</h3>
+<p>This issue can lead to bugs where conditions never evaluate as expected, causing:</p>
+<ul>
+  <li><strong>dead code paths</strong>: code that can never be reached</li>
+  <li><strong>security vulnerabilities</strong>: authorization checks that never pass or always pass</li>
+  <li><strong>logic errors</strong>: incorrect program behavior due to conditions that do not work as intended</li>
+</ul>
+<p>The severity depends on the context. An authorization check that always fails could be a critical security issue, while a redundant check might
+only affect code maintainability.</p>
+<h2>How to fix it</h2>
+<p>If the collection should contain values, populate it with the appropriate elements before performing the membership test.</p>
+<h3>Code examples</h3>
+<h4>Noncompliant code example</h4>
+<pre data-diff-id="1" data-diff-type="noncompliant">
+if user_id in []:  # Noncompliant: empty list
+    grant_access()
+</pre>
+<h4>Compliant solution</h4>
+<pre data-diff-id="1" data-diff-type="compliant">
+if user_id in ["admin", "user123", "moderator"]:
+    grant_access()
+</pre>
+<h2>Resources</h2>
+<h3>Documentation</h3>
+<ul>
+  <li>Python Documentation - <a href="https://docs.python.org/3/reference/expressions.html#membership-test-operations">Membership test
+  operations</a></li>
+  <li>Python Documentation - <a href="https://docs.python.org/3/tutorial/datastructures.html">Data Structures</a></li>
+</ul>
+<h3>Related rules</h3>
+<ul>
+  <li>{rule:python:S2583} - Conditionals should not always evaluate to the same value</li>
+  <li>{rule:python:S1145} - Useless "if(true) {…​}" and "if(false){…​}" blocks should be removed</li>
+</ul>
+

python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S8503.json

@@ -0,0 +1,25 @@
+{
+  "title": "Membership tests should not use empty collections",
+  "type": "BUG",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "5 min"
+  },
+  "tags": [
+    "pitfall",
+    "suspicious"
+  ],
+  "defaultSeverity": "Major",
+  "ruleSpecification": "RSPEC-8503",
+  "sqKey": "S8503",
+  "scope": "All",
+  "quickfix": "unknown",
+  "code": {
+    "impacts": {
+      "RELIABILITY": "MEDIUM",
+      "MAINTAINABILITY": "MEDIUM"
+    },
+    "attribute": "LOGICAL"
+  }
+}

python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_way_profile.json

@@ -332,6 +332,7 @@
     "S8493",
     "S8494",
     "S8495",
+    "S8503",
     "S8504",
     "S8505",
     "S8507",

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

@@ -0,0 +1,28 @@
+/*
+ * SonarQube Python Plugin
+ * Copyright (C) SonarSource Sàrl
+ * mailto:info AT sonarsource DOT com
+ *
+ * You can redistribute and/or modify this program under the terms of
+ * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ * See the Sonar Source-Available License for more details.
+ *
+ * You should have received a copy of the Sonar Source-Available License
+ * along with this program; if not, see https://sonarsource.com/license/ssal/
+ */
+package org.sonar.python.checks;
+
+import org.junit.jupiter.api.Test;
+import org.sonar.python.checks.utils.PythonCheckVerifier;
+
+class EmptyCollectionMembershipTestCheckTest {
+
+  @Test
+  void test() {
+    PythonCheckVerifier.verify("src/test/resources/checks/emptyCollectionMembershipTest.py", new EmptyCollectionMembershipTestCheck());
+  }
+}

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

@@ -0,0 +1,71 @@
+def empty_literals(x):
+    if x in []: ...  # Noncompliant {{Remove this membership test on an empty collection; it will always be the same value.}}
+    #  ^^^^^^^
+    if x in {}: ...  # Noncompliant
+    #  ^^^^^^^
+    if x in (): ...  # Noncompliant
+    #  ^^^^^^^
+    if x not in []: ...  # Noncompliant
+    #  ^^^^^^^^^^^
+    if x not in {}: ...  # Noncompliant
+    if x not in (): ...  # Noncompliant
+
+
+def empty_constructor_calls(x):
+    if x in set(): ...  # Noncompliant
+    #  ^^^^^^^^^^
+    if x in tuple(): ...  # Noncompliant
+    if x in frozenset(): ...  # Noncompliant
+    if x not in set(): ...  # Noncompliant
+    if x not in tuple(): ...  # Noncompliant
+    if x not in frozenset(): ...  # Noncompliant
+
+
+def outside_conditions(x, values):
+    # The rule fires on any membership test, not only those used as a condition.
+    result = x in []  # Noncompliant
+    flag = x not in set()  # Noncompliant
+    values = [y for y in range(10) if y in ()]  # Noncompliant
+
+
+def compliant_non_empty_literals(x):
+    if x in [1, 2, 3]: ...
+    if x in {"a": 1}: ...
+    if x in {1, 2}: ...
+    if x in (1,): ...
+    if x in (1, 2): ...
+    if x not in [1, 2, 3]: ...
+
+
+def compliant_constructor_calls_with_arguments(x, iterable):
+    if x in set(iterable): ...
+    if x in tuple(iterable): ...
+    if x in frozenset(iterable): ...
+    if x in list(iterable): ...
+    if x in dict(iterable): ...
+
+
+def compliant_list_dict_builtin_calls(x):
+    # list() / dict() are not covered by this rule. Use [] / {} instead.
+    if x in list(): ...
+    if x in dict(): ...
+
+
+def compliant_variables(x, values):
+    if x in values: ...
+    my_list = []
+    if x in my_list: ...  # Empty but through a variable - out of scope here
+
+
+def compliant_unpacking(x, p1, p2):
+    if x in [*p1, *p2]: ...
+    if x in {**p1}: ...
+    if x in {*p1}: ...
+    if x in (*p1,): ...
+
+
+def compliant_shadowed_builtins(x):
+    def set():  # user-defined function, shadows builtin
+        return [1, 2]
+
+    if x in set(): ...  # Not the builtin set