Modify S1121: Migrate to LayC - Assignments should not be made from w…

…ithin sub-expressions

Co-authored-by: Fred Tingaud <[email protected]>

rules/S1121/cfamily/metadata.json

@@ -1,4 +1,5 @@
 {
+  "title": "Assignments should not be made from within conditions",
   "tags": [
     "cwe",
     "based-on-misra",

rules/S1121/cfamily/rule.adoc

@@ -1,15 +1,30 @@
 == Why is this an issue?
 
-include::../description.adoc[]
+Assigning a value inside a condition (of an `if` statement, a `for` statement, a `while`, or a `switch`) can be confusing. It assigns the value and checks it at the same time, but it is easily confused with a simple equality check with `==` and the original intention can be unclear.
 
-include::../noncompliant.adoc[]
 
-include::../compliant.adoc[]
+[source,cpp,diff-id=1,diff-type=noncompliant]
+----
+  if (x = getValue()) { // Noncompliant: assigning and checking. Is it on purpose?
+    doSomething();
+  }
+----
+
+It is better to assign before the statement and use the condition for the check only:
+
+[source,cpp,diff-id=1,diff-type=compliant]
+----
+  x = getValue();
+  if (x) {
+    doSomething();
+  }
+----
 
 === Exceptions
 
-Assignments explicitly enclosed in parentheses are ignored.
+This rule ignores assignments explicitly enclosed in parentheses.
 
+[source,cpp]
 ----
 while ((run = keepRunning())) {
   //...
@@ -18,12 +33,19 @@ while ((run = keepRunning())) {
 
 == Resources
 
+=== Documentation
+
+* CWE - https://cwe.mitre.org/data/definitions/481[481: Assigning instead of Comparing]
+
+=== Standards
+
+* CERT - https://wiki.sei.cmu.edu/confluence/x/ZNYxBQ[EXP45-C. Do not perform assignments in selection statements]
+
+=== External coding guidelines
+
 * MISRA C:2004, 13.1 - Assignment operators shall not be used in expressions that yield a Boolean value
 * MISRA {cpp}:2008, 6-2-1 - Assignment operators shall not be used in sub-expressions
 * MISRA C:2012, 13.4 - The result of an assignment operator should not be used
-* https://cwe.mitre.org/data/definitions/481[MITRE, CWE-481] - Assigning instead of Comparing
-* https://wiki.sei.cmu.edu/confluence/x/ZNYxBQ[CERT, EXP45-C.] - Do not perform assignments in selection statements
-* https://wiki.sei.cmu.edu/confluence/x/ITZGBQ[CERT, EXP51-J.] - Do not perform assignments in conditional expressions
 
 ifdef::env-github,rspecator-view[]
 

rules/S1121/csharp/rule.adoc

@@ -2,39 +2,31 @@
 
 include::../description.adoc[]
 
-=== Noncompliant code example
-
-[source,csharp]
-----
-if (string.IsNullOrEmpty(result = str.Substring(index, length))) // Noncompliant
-{
-  //...
-}
-----
+=== Exceptions
 
-=== Compliant solution
+Assignments inside lambda and delegate expressions are allowed.
 
 [source,csharp]
 ----
-var result = str.Substring(index, length);
-if (string.IsNullOrEmpty(result))
+var result = Foo(() =>
 {
-  //...
+   int x = 100; // dead store, but ignored
+   x = 200;
+   return x;
 }
 ----
 
-=== Exceptions
-
-Assignments inside lambda and delegate expressions are allowed. 
+The rule also ignores the following patterns:
 
-
-Furthermore, the following patterns are also accepted:
+* Chained assignments
 
 [source,csharp]
 ----
 var a = b = c = 10;
 ----
 
+* Assignments that are part of a condition of an `if` statement or a loop
+
 [source,csharp]
 ----
 while ((val = GetNewValue()) > 0)
@@ -43,15 +35,38 @@ while ((val = GetNewValue()) > 0)
 }
 ----
 
+* Assignment in the right-hand side of a coalescing operator
+
 [source,csharp]
 ----
 private MyClass instance;
-public MyClass Instance
+public MyClass Instance => instance ?? (instance = new MyClass());
+----
+
+== How to fix it
+
+include::../how-to-fix-it.adoc[]
+
+=== Code examples
+
+==== Noncompliant code example
+
+[source,csharp,diff-id=1,diff-type=noncompliant]
+----
+if (string.IsNullOrEmpty(result = str.Substring(index, length))) // Noncompliant
+{
+  // do something with "result"
+}
+----
+
+==== Compliant solution
+
+[source,csharp,diff-id=1,diff-type=compliant]
+----
+var result = str.Substring(index, length);
+if (string.IsNullOrEmpty(result))
 {
-  get
-  {
-    return instance ?? (instance = new MyClass());
-  }
+  // do something with "result"
 }
 ----
 

rules/S1121/description.adoc

@@ -1 +1,4 @@
-Assignments within sub-expressions are hard to spot and therefore make the code less readable. Ideally, sub-expressions should not have side-effects.
+A common code smell that can hinder the clarity of source code is making assignments within sub-expressions.
+This practice involves assigning a value to a variable inside a larger expression, such as within a loop or a conditional statement.
+
+This practice essentially gives a side-effect to a larger expression, thus making it less readable. This often leads to confusion and potential errors.

rules/S1121/how-to-fix-it.adoc

@@ -0,0 +1,5 @@
+Making assignments within sub-expressions can hinder the clarity of source code.
+
+This practice essentially gives a side-effect to a larger expression, thus making it less readable. This often leads to confusion and potential errors.
+
+Extracting assignments into separate statements is encouraged to keep the code clear and straightforward.

rules/S1121/java/rule.adoc

@@ -2,33 +2,63 @@
 
 include::../description.adoc[]
 
-include::../noncompliant.adoc[]
-
-include::../compliant.adoc[]
-
 === Exceptions
 
-Assignments in ``++while++`` statement conditions, and assignments enclosed in relational expressions are ignored.
+This rule ignores assignments in conditions of `while` statements and assignments enclosed in relational expressions.
 
+[source,java]
 ----
-BufferedReader br = new BufferedReader(/* ... */);
-String line;
-while ((line = br.readLine()) != null) {...}
-if ((i = j) >= 1) {...}
+void processInput(BufferedReader br) {
+  String line;
+  while ((line = br.readLine()) != null) {
+    processLine(line);
+  }
+}
+
+Object foo;
+if ((foo = bar()) != null) {
+  // do something with "foo"
+}
 ----
 
-Chained assignments, including compound assignments, are ignored.
+This rule also ignores chained assignments, including compound assignments.
 
+[source,java]
 ----
-int i = j = 0;
+int j, i = j = 0;
 int k = (j += 1);
+byte[] result, bresult;
 result = (bresult = new byte[len]);
 ----
 
-== Resources
+== How to fix it
+
+include::../how-to-fix-it.adoc[]
+
+=== Code examples
+
+==== Noncompliant code example
+
+[source,java,diff-id=1,diff-type=noncompliant]
+----
+String str;
+if (!(str = cont.substring(pos1, pos2)).isEmpty()) {  // Noncompliant
+  // do something with "str"
+}
+----
+
+==== Compliant solution
+
+[source,java,diff-id=1,diff-type=compliant]
+----
+String str = cont.substring(pos1, pos2);
+if (!str.isEmpty()) {
+  // do something with "str"
+}
+----
+
+include::../see.adoc[]
 
-* https://cwe.mitre.org/data/definitions/481[MITRE, CWE-481] - Assigning instead of Comparing
-* https://wiki.sei.cmu.edu/confluence/x/ZNYxBQ[CERT, EXP45-C.] - Do not perform assignments in selection statements
 * https://wiki.sei.cmu.edu/confluence/x/ITZGBQ[CERT, EXP51-J.] - Do not perform assignments in conditional expressions
 
 ifdef::env-github,rspecator-view[]

rules/S1121/javascript/rule.adoc

@@ -2,39 +2,44 @@
 
 include::../description.adoc[]
 
-Moreover, using chained assignment in declarations is also dangerous because one may accidentally create global variables, e.g. in ``++let x = y = 1;++``, if ``++y++`` is not declared, it will be hoisted as global.
+Moreover, using chained assignments in declarations is also dangerous because one may accidentally create global variables. Consider the following code snippet: ``++let x = y = 1;++``. If ``++y++`` is not declared, it will be hoisted as global.
 
-=== Noncompliant code example
+=== Exceptions
+
+The rule does not raise issues for the following patterns:
+
+* chained assignments: ``++a = b = c = 0;++``
+* relational assignments: ``++(a = 0) != b++``
+* sequential assignments: ``++a = 0, b = 1, c = 2++``
+* assignments in lambda body: ``++() => a = 0++``
+* conditional assignment idiom: ``++a || (a = 0)++``
+* assignments in (do-)while conditions: ``++while (a = 0);++``
+
+== How to fix it
+
+include::../how-to-fix-it.adoc[]
+
+=== Code examples
 
-[source,javascript]
+==== Noncompliant code example
+
+[source,javascript,diff-id=1,diff-type=noncompliant]
 ----
 if (val = value() && check()) { // Noncompliant
   // ...
 }
 ----
 
-=== Compliant solution
+==== Compliant solution
 
-[source,javascript]
+[source,javascript,diff-id=1,diff-type=compliant]
 ----
 val = value();
 if (val && check()) {
   // ...
 }
 ----
 
-=== Exceptions
-
-The rule does not raise issues for the following patterns:
-
-
-* chained assignments: ``++a = b = c = 0;++``
-* relational assignments: ``++(a = 0) != b++``
-* sequential assignments: ``++a = 0, b = 1, c = 2++``
-* assignments in lambda body: ``++() => a = 0++``
-* conditional assignment idiom: ``++a || (a = 0)++``
-* assignments in (do-)while conditions: ``++while (a = 0);++``
-
 include::../see.adoc[]
 
 ifdef::env-github,rspecator-view[]

rules/S1121/php/rule.adoc

@@ -2,41 +2,47 @@
 
 include::../description.adoc[]
 
-=== Noncompliant code example
+=== Exceptions
+
+This rule ignores assignments in conditions of `while` statements and assignments enclosed in relational expressions.
 
 [source,php]
 ----
+while (($line = next_line()) != NULL) {...}
+
+while ($line = next_line()) {...}
+----
+
+== How to fix it
+
+include::../how-to-fix-it.adoc[]
+
+=== Code examples
+
+==== Noncompliant code example
+
+[source,php,diff-id=1,diff-type=noncompliant]
+----
 if (($val = value()) && check()) { // Noncompliant
 }
 ----
 
-=== Compliant solution
+==== Compliant solution
 
-[source,php]
+[source,php,diff-id=1,diff-type=compliant]
 ----
 $val = value();
 if ($val && check()) {
 }
 ----
 or 
 
-[source,php]
+[source,php,diff-id=1,diff-type=compliant]
 ----
-if ($val == value() && check()) { // Perhaps in fact the equality operator was expected
+if ($val == value() && check()) { // Original intention might have been to use equality operator and not assignment
 }
 ----
 
-=== Exceptions
-
-Assignments in ``++while++`` statement conditions, and assignments enclosed in relational expressions are allowed.
-
-[source,php]
-----
-while (($line = next_line()) != NULL) {...}
-
-while ($line = next_line()) {...}
-----
-
 include::../see.adoc[]
 
 ifdef::env-github,rspecator-view[]