diff --git a/rules/S1121/cfamily/metadata.json b/rules/S1121/cfamily/metadata.json index 7b2b18f9749..33ef5b7af31 100644 --- a/rules/S1121/cfamily/metadata.json +++ b/rules/S1121/cfamily/metadata.json @@ -1,4 +1,5 @@ { + "title": "Assignments should not be made from within conditions", "tags": [ "cwe", "based-on-misra", diff --git a/rules/S1121/cfamily/rule.adoc b/rules/S1121/cfamily/rule.adoc index adc8a40c955..2914061eb05 100644 --- a/rules/S1121/cfamily/rule.adoc +++ b/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[] diff --git a/rules/S1121/compliant.adoc b/rules/S1121/compliant.adoc deleted file mode 100644 index c2cd02befd2..00000000000 --- a/rules/S1121/compliant.adoc +++ /dev/null @@ -1,8 +0,0 @@ -=== Compliant solution - -[source,text] ----- -str = cont.substring(pos1, pos2); -if (str.isEmpty()) { - //... ----- diff --git a/rules/S1121/csharp/rule.adoc b/rules/S1121/csharp/rule.adoc index b4bffcf7abb..e3b96392576 100644 --- a/rules/S1121/csharp/rule.adoc +++ b/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" } ---- diff --git a/rules/S1121/description.adoc b/rules/S1121/description.adoc index 84f5ffae32d..88665af2f54 100644 --- a/rules/S1121/description.adoc +++ b/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. diff --git a/rules/S1121/how-to-fix-it.adoc b/rules/S1121/how-to-fix-it.adoc new file mode 100644 index 00000000000..77488471868 --- /dev/null +++ b/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. \ No newline at end of file diff --git a/rules/S1121/java/rule.adoc b/rules/S1121/java/rule.adoc index 1897f15fc5f..7824c60a297 100644 --- a/rules/S1121/java/rule.adoc +++ b/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[] diff --git a/rules/S1121/javascript/rule.adoc b/rules/S1121/javascript/rule.adoc index e3f3f1a6e30..2d58f7bbbac 100644 --- a/rules/S1121/javascript/rule.adoc +++ b/rules/S1121/javascript/rule.adoc @@ -2,20 +2,37 @@ 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()) { @@ -23,18 +40,6 @@ 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[] diff --git a/rules/S1121/noncompliant.adoc b/rules/S1121/noncompliant.adoc deleted file mode 100644 index e1dcee73561..00000000000 --- a/rules/S1121/noncompliant.adoc +++ /dev/null @@ -1,7 +0,0 @@ -=== Noncompliant code example - -[source,text] ----- -if ((str = cont.substring(pos1, pos2)).isEmpty()) { // Noncompliant - //... ----- diff --git a/rules/S1121/php/rule.adoc b/rules/S1121/php/rule.adoc index 8d4aadcd09a..04e2efa2789 100644 --- a/rules/S1121/php/rule.adoc +++ b/rules/S1121/php/rule.adoc @@ -2,17 +2,34 @@ 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()) { @@ -20,23 +37,12 @@ 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[] diff --git a/rules/S1121/rule.adoc b/rules/S1121/rule.adoc deleted file mode 100644 index 7f48b2b9fd7..00000000000 --- a/rules/S1121/rule.adoc +++ /dev/null @@ -1,9 +0,0 @@ -== Why is this an issue? - -include::description.adoc[] - -include::noncompliant.adoc[] - -include::compliant.adoc[] - -include::see.adoc[]