From 66d4fe3a0d7e0001fd83a262121665cb6182038c Mon Sep 17 00:00:00 2001 From: Philipp Dominik Schubert <119606487+pdschbrt@users.noreply.github.com> Date: Thu, 28 Sep 2023 06:25:53 -0700 Subject: [PATCH] Modify rule S1854: Expand and adjust for LaYC ## Review A dedicated reviewer checked the rule description successfully for: - [ ] logical errors and incorrect information - [ ] information gaps and missing content - [ ] text style and tone - [ ] PR summary and labels follow [the guidelines](https://github.com/SonarSource/rspec/#to-modify-an-existing-rule) --------- Co-authored-by: Arseniy Zaostrovnykh --- rules/S1854/cfamily/rule.adoc | 192 +++++++++++++++++++++++++++++++--- rules/S1854/metadata.json | 2 +- 2 files changed, 181 insertions(+), 13 deletions(-) diff --git a/rules/S1854/cfamily/rule.adoc b/rules/S1854/cfamily/rule.adoc index 1380f8da3f3..515bd5bf0b8 100644 --- a/rules/S1854/cfamily/rule.adoc +++ b/rules/S1854/cfamily/rule.adoc @@ -1,25 +1,193 @@ +Unused assignments should be removed. + == Why is this an issue? -include::../description.adoc[] +Computing or retrieving a value only to then immediately overwrite it or throw it away indicates a serious logic error in the code. + +Assigning a value to a local variable that is not read by any subsequent instruction is called a _dead store_. +The following code snippet depicts a few dead stores. + +[source,cpp] +---- +int foo() { + int x = 0; // Noncompliant: dead store, next line overwrites x + x = 100; // Noncompliant: dead store, next line overwrites x + x = 200; + int y = 0; + y += 9001; // Noncompliant: dead store, y is never used + int z = 300; // Noncompliant: dead store, next line overwrites z + z = 400; + return x + z * 2; +} +---- + +Even if the unnecessary operations do not do any harm in terms of the program's correctness, they are--at best--a waste of computing resources. +In most cases, these operations have their intended use but it is not expressed correctly in the code. +Therefore, unused values and superfluous code should be removed to prevent logic errors. + + +== What is the potential impact? + +Not only do unused values and superfluous code make the program unnecessary complex, but also indicate significant logic errors. +And even in the absence of logic errors, they waste computing resources in case the compiler is not able to optimize them away. + +Unused values typically showcase a discrepancy between what a developer intended and what is specified in the code and should be removed to uncover and eventually prevent logic errors. + + +== How to fix it + +Remove unused values and superfluous code. + + +=== Code examples + +==== Noncompliant code example + +[source,cpp,diff-id=1,diff-type=noncompliant] +---- +int foo(int y) { + int x = 0; + x = 100; // Noncompliant: dead store + x = 200; + return x + y; +} +---- + +==== Compliant solution + +[source,cpp,diff-id=1,diff-type=compliant] +---- +int foo(int y) { + int x = 200; // Compliant: no unnecessary assignment + return x + y; +} +---- + +==== Noncompliant code example + +[source,cpp,diff-id=2,diff-type=noncompliant] +---- +int bar(); +int buz(); + +int foo(bool b) { + int x = 0; + if (b) { + x = bar(); + return x; + } + if (x != 0) { + int y = buz(); + y += 9001; // Noncompliant: dead store + } + return x; +} +---- -include::../noncompliant.adoc[] +==== Compliant solution -include::../compliant.adoc[] +[source,cpp,diff-id=2,diff-type=compliant] +---- +int bar(); +int buz(); -=== Exceptions +int foo(bool b) { + int x = 0; + if (b) { + x = bar(); + return x; + } + // Compliant: no more dead stores and superfluous code + // Assuming call to buz() had no important side effects + return x; +} +---- -This rule ignores: -* variable declarations initializers -* prefix and postfix increments and decrements ``x{plus}{plus};`` -* null pointer assignments ``++x = NULL;++`` -* self assignments (i.e. ``++x = x;++``) +=== Pitfalls + +When removing unused values and superfluous code, make sure that the right-hand side of a given assignment has no side effects. + +While it is safe to remove the call to `square` in the following code since it has no side effects, removing the call to `fwrite` changes the program's behavior. +Still, values that are never read such as `n` indicate code smells that should be mitigated. +In this code example, the return value of `fwrite` should be checked and any potential error should be handled appropriately. + +[source,cpp] +---- +#include +#include + +int square(int n) { + return n * n; +} + +int foo(int i) { + int sq = square(i); // Noncompliant: dead store, assignment can be removed + const char* const str = "Hello, World!\n"; + // Although `n` is never read, the call to `fwrite` cannot be removed due to side effects + size_t n = fwrite(str, sizeof(char), strlen(str), stdout); // Noncompliant: `n` is never read + return i + 9001; +} +---- + + +=== Going the extra mile + +In {cpp}17, the `nodiscard` attribute has been introduced which can be used to annotate functions, enumerations and classes. + +The attribute serves as a hint to the compiler and to other developers that a function's return value should not be ignored. +A function that is marked `nodiscard` whose return value is ignored encourages the compiler to issue a warning. +Example usages of the `nodiscard` attribute are shown in the following: + +[source,cpp] +---- +[[nodiscard]] int foo() { return 100; } +int bar() { return 200; } +[[nodiscard("An explanation on why not to discard the return value")]] int buz() { return 300; } + +enum class [[nodiscard]] important_error_info { OK, WARN, CRITICAL }; +important_error_info compute() { + // More code ... + // In case of a critical error, return corresponding error info: + return important_error_info::CRITICAL; +} + +void caller() { + foo(); // compiler warns on discarding a nodiscard value + bar(); // compiler will issue no warning + buz(); // compiler warns on discarding a nodiscard value + compute(); // compiler warns on discarding a nodiscard value +} +---- + +In case, the return value of a function marked as `nodiscard` should be (exceptionally) ignored, a cast to `void` can be used to silence the compiler warning as shown in the following: + +[source,cpp] +---- +[[nodiscard]] int foo() { return 100; } + +void caller() { + foo(); // compiler warns on discarding a nodiscard value + (void)foo(); // compiler will issue no warning +} +---- == Resources -* https://cwe.mitre.org/data/definitions/563[MITRE, CWE-563] - Assignment to Variable without Use ('Unused Variable') -* https://wiki.sei.cmu.edu/confluence/x/39UxBQ[CERT, MSC13-C.] - Detect and remove unused values -* https://wiki.sei.cmu.edu/confluence/x/9DZGBQ[CERT, MSC56-J.] - Detect and remove superfluous code and values +=== Standards + +* CERT - https://wiki.sei.cmu.edu/confluence/x/39UxBQ[MSC13-C. Detect and remove unused values] +* CERT - https://wiki.sei.cmu.edu/confluence/x/9DZGBQ[MSC56-J. Detect and remove superfluous code and values] +* CWE - https://cwe.mitre.org/data/definitions/563[563 - Assignment to Variable without Use ('Unused Variable')] + +=== Related rules + +* S1763 detects unreachable code +* S2583 ensures that conditionally executed code is reachable +* S2589 detects gratuitous boolean expressions +* S3516 ensures that a function returns non-invariant +* S3626 identifies redundant jump statements + ifdef::env-github,rspecator-view[] diff --git a/rules/S1854/metadata.json b/rules/S1854/metadata.json index 88dce851cc1..3b6605f536d 100644 --- a/rules/S1854/metadata.json +++ b/rules/S1854/metadata.json @@ -36,5 +36,5 @@ "defaultQualityProfiles": [ "Sonar way" ], - "quickfix": "unknown" + "quickfix": "targeted" }