SONARPY-5012 S9409: Only raise on for-loops and name the iterated collection

GitOrigin-RevId: d727bfea6e2b112da6b08f9e6cdcb9a62f9882a0

Author: rombirli

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

@@ -36,6 +36,5 @@ public class ListExtendOverIndividualAppendsCheck extends PythonSubscriptionChec
   @Override
   public void initialize(Context context) {
     context.registerSyntaxNodeConsumer(Tree.Kind.FOR_STMT, ctx -> CollectionBulkAdditionUtils.checkForStatement(ctx, LIST_EXTEND));
-    context.registerSyntaxNodeConsumer(Tree.Kind.STATEMENT_LIST, ctx -> CollectionBulkAdditionUtils.checkStatementList(ctx, LIST_EXTEND));
   }
 }

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

@@ -46,7 +46,8 @@ public final class CollectionBulkAdditionUtils {
 
   private static final int MIN_CONSECUTIVE_ADDITIONS = 3;
 
-  private static final String FOR_LOOP_MESSAGE = "Use \"%s.%s()\" instead of a for-loop with \"%s()\".";
+  private static final String FOR_LOOP_NAMED_MESSAGE = "Use \"%s.%s()\" to add all elements of \"%s\" instead of a for-loop with \"%s()\".";
+  private static final String FOR_LOOP_MESSAGE = "Use \"%s.%s()\" to add all elements of the iterated collection instead of a for-loop with \"%s()\".";
   private static final String CONSECUTIVE_MESSAGE = "Use \"%s.%s()\" instead of consecutive \"%s()\" calls.";
   private static final String SECONDARY_MESSAGE = "This call is part of the same sequence.";
 
@@ -77,6 +78,7 @@ public static void checkForStatement(SubscriptionContext ctx, BulkAddition bulkA
       return;
     }
     Expression iterable = forStatement.testExpressions().get(0);
+    Name iterableName = Expressions.removeParentheses(iterable) instanceof Name name ? name : null;
 
     CallExpression call = singleBodyCall(forStatement);
     if (call == null || !(call.callee() instanceof QualifiedExpression callee)) {
@@ -99,7 +101,7 @@ public static void checkForStatement(SubscriptionContext ctx, BulkAddition bulkA
       return;
     }
 
-    ctx.addIssue(callee, message(FOR_LOOP_MESSAGE, bulkAddition));
+    ctx.addIssue(callee, forLoopMessage(bulkAddition, iterableName));
   }
 
   public static void checkStatementList(SubscriptionContext ctx, BulkAddition bulkAddition) {
@@ -153,6 +155,13 @@ private static void reportRun(SubscriptionContext ctx, BulkAddition bulkAddition
     run.stream().skip(1).forEach(addition -> issue.secondary(addition.call(), SECONDARY_MESSAGE));
   }
 
+  private static String forLoopMessage(BulkAddition bulkAddition, @Nullable Name collection) {
+    if (collection != null) {
+      return String.format(FOR_LOOP_NAMED_MESSAGE, bulkAddition.typeName(), bulkAddition.bulkMethod(), collection.name(), bulkAddition.elementMethod());
+    }
+    return message(FOR_LOOP_MESSAGE, bulkAddition);
+  }
+
   private static String message(String template, BulkAddition bulkAddition) {
     return String.format(template, bulkAddition.typeName(), bulkAddition.bulkMethod(), bulkAddition.elementMethod());
   }

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

@@ -1,78 +1,53 @@
-<p>This rule raises an issue when several elements are added to the same list through consecutive <code>append()</code> calls, either as a sequence of
-statements or through a for-loop.</p>
+<p>This rule raises an issue when a for-loop adds every element of a collection to a list through <code>append()</code>.</p>
 <h2>Why is this an issue?</h2>
-<p>Adding elements to a list one at a time is unnecessary when the <code>extend()</code> method exists. This happens with a sequence of
-<code>append()</code> statements:</p>
-<pre>
-my_list.append(first)
-my_list.append(second)
-my_list.append(third)
-</pre>
-<p>It also happens when a for-loop appends every element of another collection, which is the same sequence of calls spread over an iteration:</p>
+<p>Adding elements to a list one at a time is unnecessary when the <code>extend()</code> method exists. This happens when a for-loop appends every
+element of another collection:</p>
 <pre>
 for element in other_collection:
     my_list.append(element)
 </pre>
-<p>The <code>extend()</code> method combines all additions into a single call and avoids the overhead of repeated <code>append()</code>
-invocations:</p>
+<p>The <code>extend()</code> method adds all elements in a single call and avoids the overhead of repeated <code>append()</code> invocations:</p>
 <pre>
-my_list.extend([first, second, third])
 my_list.extend(other_collection)
 </pre>
 <h3>What is the potential impact?</h3>
-<p>Grouping the additions makes the intent explicit: the reader sees one operation that grows the list instead of a series of independent statements.
+<p>Replacing the loop makes the intent explicit: the reader sees one operation that grows the list instead of an iteration whose body has to be read.
 It also removes redundant call overhead, since the attribute lookup and the method call are resolved once instead of once per element. In hot code or
 when the source collection is large, the difference becomes measurable.</p>
 <h3>Exceptions</h3>
-<p>No issue is raised when other statements are interleaved between the <code>append()</code> calls, or when the body of the loop does more than
-append the loop variable, for example when it filters or transforms the elements:</p>
+<p>No issue is raised when the body of the loop does more than append the loop variable, for example when it filters or transforms the elements:</p>
 <pre>
 for element in other_collection:
     if element.is_valid():
         my_list.append(element)
 </pre>
 <h2>How to fix it</h2>
-<p>Group the appended elements into a single <code>extend()</code> call. For consecutive statements, pass the elements as a list literal. For a loop,
-pass the iterated collection directly, as <code>extend()</code> accepts any iterable.</p>
+<p>Replace the loop with a single <code>extend()</code> call and pass the iterated collection directly, as <code>extend()</code> accepts any
+iterable.</p>
 <h3>Code examples</h3>
 <h4>Noncompliant code example</h4>
 <pre data-diff-id="1" data-diff-type="noncompliant">
-def build_command(base_command, path):
-    command = list(base_command)
-    command.append("--archive")  # Noncompliant
-    command.append("--verbose")
-    command.append(path)
-    return command
-</pre>
-<h4>Compliant solution</h4>
-<pre data-diff-id="1" data-diff-type="compliant">
-def build_command(base_command, path):
-    command = list(base_command)
-    command.extend(["--archive", "--verbose", path])
-    return command
-</pre>
-<h4>Noncompliant code example</h4>
-<pre data-diff-id="2" data-diff-type="noncompliant">
 def collect_results(results, new_results):
     for result in new_results:
         results.append(result)  # Noncompliant
     return results
 </pre>
 <h4>Compliant solution</h4>
-<pre data-diff-id="2" data-diff-type="compliant">
+<pre data-diff-id="1" data-diff-type="compliant">
 def collect_results(results, new_results):
     results.extend(new_results)
     return results
 </pre>
 <h3>How does this work?</h3>
-<p><code>extend()</code> iterates over its argument and appends every element to the list, so the result is exactly the same as the individual calls.
-The augmented assignment <code>my_list += [first, second, third]</code> is equivalent to <code>extend()</code> and can be used instead when it reads
-better.</p>
+<p><code>extend()</code> iterates over its argument and appends every element to the list, so the result is exactly the same as the loop. The
+augmented assignment <code>my_list += other_collection</code> is equivalent to <code>extend()</code> and can be used instead when it reads better.</p>
 <h3>Pitfalls</h3>
 <p>Do not confuse <code>extend()</code> with <code>append()</code>: <code>my_list.append(other_collection)</code> adds the collection itself as a
 single element, while <code>extend()</code> adds its elements.</p>
-<p>A loop that appends to the list it iterates over never terminates, while <code>my_list.extend(my_list)</code> simply doubles the list. The fix is
-behavior-preserving in every other case, so make sure the loop you replace iterates over a different collection.</p>
+<p>A loop that appends to the list it iterates over never terminates, while <code>my_list.extend(my_list)</code> simply doubles the list. Make sure
+the loop you replace iterates over a different collection.</p>
+<p>After a for-loop, the loop variable stays bound to the last element, while <code>extend()</code> defines no variable. If code after the loop reads
+that variable, keep it defined before replacing the loop.</p>
 <h2>Resources</h2>
 <h3>Documentation</h3>
 <ul>
@@ -82,6 +57,7 @@ <h3>Documentation</h3>
 </ul>
 <h3>Related rules</h3>
 <ul>
-  <li>{rule:python:S8502} - "set.update()" should be used instead of consecutive "add()" calls</li>
+  <li>{rule:python:S8502} - "set.update()" should be used instead of a for-loop with "add()"</li>
+  <li>{rule:python:S9454} - "list.extend()" should be used instead of consecutive "append()" calls</li>
 </ul>
 

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

@@ -1,5 +1,5 @@
 {
-  "title": "\"list.extend()\" should be used instead of consecutive \"append()\" calls",
+  "title": "\"list.extend()\" should be used instead of a for-loop with \"append()\"",
   "type": "CODE_SMELL",
   "status": "ready",
   "remediation": {

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

@@ -3,7 +3,7 @@ def noncompliant_for_loop():
     my_list = [1, 2, 3]
     other_collection = [4, 5, 6]
     for element in other_collection:
-        my_list.append(element)  # Noncompliant {{Use "list.extend()" instead of a for-loop with "append()".}}
+        my_list.append(element)  # Noncompliant {{Use "list.extend()" to add all elements of "other_collection" instead of a for-loop with "append()".}}
     #   ^^^^^^^^^^^^^^
 
 
@@ -14,7 +14,7 @@ def noncompliant_for_loop_multiline_iterable():
         2,
         3,
     ]:
-        my_list.append(element)  # Noncompliant
+        my_list.append(element)  # Noncompliant {{Use "list.extend()" to add all elements of the iterated collection instead of a for-loop with "append()".}}
     #   ^^^^^^^^^^^^^^
 
 
@@ -26,57 +26,46 @@ def noncompliant_for_loop_multiline_receiver():
          ).append(item)
 
 
-def noncompliant_consecutive_appends(base_command, path):
-    command = list(base_command)
-    command.append("--archive")  # Noncompliant {{Use "list.extend()" instead of consecutive "append()" calls.}}
-#   ^^^^^^^^^^^^^^
-    command.append("--verbose")
-#   ^^^^^^^^^^^^^^< {{This call is part of the same sequence.}}
-    command.append(path)
-#   ^^^^^^^^^^^^^^< {{This call is part of the same sequence.}}
-    return command
+def noncompliant_for_loop_named_iterable_message(new_results):
+    results = []
+    for result in new_results:
+        results.append(result)  # Noncompliant {{Use "list.extend()" to add all elements of "new_results" instead of a for-loop with "append()".}}
+    #   ^^^^^^^^^^^^^^
 
 
-def noncompliant_consecutive_appends_four_calls():
-    my_list = []
-    my_list.append(1)  # Noncompliant
-    my_list.append(2)
-    my_list.append(3)
-    my_list.append(4)
+def noncompliant_for_loop_parenthesized_iterable_message(new_results):
+    results = []
+    for result in (new_results):
+        results.append(result)  # Noncompliant {{Use "list.extend()" to add all elements of "new_results" instead of a for-loop with "append()".}}
+    #   ^^^^^^^^^^^^^^
 
 
-def noncompliant_consecutive_appends_complex_arguments(items, flag):
+def noncompliant_for_loop_call_iterable_message():
     my_list = []
-    my_list.append(items[0])  # Noncompliant
-    my_list.append(items[1] if flag else None)
-    my_list.append(len(items))
+    for i in range(10):
+        my_list.append(i)  # Noncompliant {{Use "list.extend()" to add all elements of the iterated collection instead of a for-loop with "append()".}}
+    #   ^^^^^^^^^^^^^^
 
 
-def noncompliant_consecutive_appends_after_other_statements():
+def noncompliant_for_loop_attribute_iterable_message(config):
     my_list = []
-    print("start")
-    my_list.append(1)  # Noncompliant
-    my_list.append(2)
-    my_list.append(3)
+    for path in config.paths:
+        my_list.append(path)  # Noncompliant {{Use "list.extend()" to add all elements of the iterated collection instead of a for-loop with "append()".}}
+    #   ^^^^^^^^^^^^^^
 
 
-def noncompliant_consecutive_appends_two_runs(other_list):
+def noncompliant_for_loop_subscription_iterable_message(groups):
     my_list = []
-    my_list.append(1)  # Noncompliant
-    my_list.append(2)
-    my_list.append(3)
-    print("separator")
-    my_list.append(4)  # Noncompliant
-    my_list.append(5)
-    my_list.append(6)
+    for item in groups[0]:
+        my_list.append(item)  # Noncompliant {{Use "list.extend()" to add all elements of the iterated collection instead of a for-loop with "append()".}}
+    #   ^^^^^^^^^^^^^^
 
 
 class MyClass:
-    def build(self, path):
+    def build(self, items):
         my_list = []
-        my_list.append("first")  # Noncompliant
-        my_list.append("second")
-        my_list.append(path)
+        for item in items:
+            my_list.append(item)  # Noncompliant
         return my_list
 
 
@@ -87,91 +76,19 @@ def compliant_extend_call():
     my_list.extend(["a", "b", "c"])
 
 
-def compliant_two_consecutive_appends():
-    my_list = []
-    my_list.append(1)
-    my_list.append(2)
-
-
-def compliant_appends_interleaved_with_other_statements():
-    my_list = []
-    my_list.append(1)
-    print("in between")
-    my_list.append(2)
-    print("in between")
-    my_list.append(3)
-
-
-def compliant_appends_on_different_lists():
-    first = []
-    second = []
-    third = []
-    first.append(1)
-    second.append(2)
-    third.append(3)
-
-
-def compliant_appends_referencing_the_list(items):
-    # Grouping these calls would change the result: each argument observes the previous append
-    my_list = []
-    my_list.append(len(my_list))
-    my_list.append(len(my_list))
-    my_list.append(len(my_list))
-
-
-def compliant_appends_in_different_blocks(flag):
-    my_list = []
-    if flag:
-        my_list.append(1)
-    else:
-        my_list.append(2)
-    my_list.append(3)
-
-
-def compliant_appends_on_unknown_receiver(unknown_receiver):
-    unknown_receiver.append(1)
-    unknown_receiver.append(2)
-    unknown_receiver.append(3)
+def compliant_consecutive_appends(base_command, path):
+    # Consecutive calls are covered by S9454
+    command = list(base_command)
+    command.append("--archive")
+    command.append("--verbose")
+    command.append(path)
+    return command
 
 
 def get_list():
     return []
 
 
-def compliant_appends_on_transient_receiver():
-    get_list().append(1)
-    get_list().append(2)
-    get_list().append(3)
-
-
-def compliant_appends_on_a_set():
-    my_set = {1}
-    my_set.add(1)
-    my_set.add(2)
-    my_set.add(3)
-
-
-def compliant_appends_with_wrong_arity():
-    my_list = []
-    my_list.append()
-    my_list.append(1, 2)
-    my_list.append(3)
-
-
-def compliant_appends_with_keyword_argument():
-    my_list = []
-    my_list.append(item=1)
-    my_list.append(item=2)
-    my_list.append(item=3)
-
-
-def compliant_appends_with_starred_argument(args):
-    my_list = []
-    my_list.append(*args)
-    my_list.append(*args)
-    my_list.append(*args)
-
-
 def compliant_for_loop_tuple_unpacking_loop_var():
     my_list = []
     pairs = [(1, "a"), (2, "b")]

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

@@ -3,7 +3,7 @@ def noncompliant_basic():
     my_set = {1, 2, 3}
     other_collection = [4, 5, 6]
     for element in other_collection:
-        my_set.add(element)  # Noncompliant {{Use "set.update()" instead of a for-loop with "add()".}}
+        my_set.add(element)  # Noncompliant {{Use "set.update()" to add all elements of "other_collection" instead of a for-loop with "add()".}}
     #   ^^^^^^^^^^