diff --git a/rules/S2589/python/rule.adoc b/rules/S2589/python/rule.adoc index 361abca7b55..ac9af47765d 100644 --- a/rules/S2589/python/rule.adoc +++ b/rules/S2589/python/rule.adoc @@ -1,10 +1,48 @@ + +Gratuitous boolean expressions are conditions that do not change the evaluation +of a program. +This issue can indicate logical errors and affect the correctness of an +application, as well as its maintainability. + == Why is this an issue? -include::../description.adoc[] +Control flow constructs like `if`-statements allow the programmer to direct the +flow of a program depending on a boolean expression. +However, if the condition is always true or always false, only one of the +branches will ever be executed. +In that case, the control flow construct and the condition no longer serve a +purpose; they become _gratuitous_. + +=== What is the potential impact? + +The presence of gratuitous conditions can indicate a logical error. +I.e., the programmer _did intend_ to have the program branch into different +paths but they made a slight mistake when formulating the branching condition. +In this case, this issue might result in a bug and thus affect the reliability +of the application. +For instance, it might lead to the computation of incorrect results. + +Additionally, gratuitous conditions and control flow constructs introduce +unnecessary complexity. +The source code becomes harder to understand, and thus, the application becomes +more difficult to maintain. + +== How to fix it -=== Noncompliant code example +First, the boolean expression in question should be closely inspected for +logical errors. +If a mistake was made, it can be corrected so that the condition is no longer +gratuitous. -[source,python] +If it becomes apparent that the condition is actually unnecessary, +it can be removed together with the associated control flow construct +(e.g. the `if`-statement containing the condition). + +=== Code examples + +==== Noncompliant code example + +[source,python,diff-id=1,diff-type=compliant] ---- def f(b): a = True @@ -12,21 +50,62 @@ def f(b): do_something() if a and b: # Noncompliant; "a" is always "True" - do_something() + do_something_else() ---- -=== Compliant solution +==== Compliant solution -[source,python] +[source,python,diff-id=1,diff-type=noncompliant] ---- def f(b): a = True - if foo(a): - do_something() + do_something() if b: + do_something_else() +---- + +==== Noncompliant code example + +[source,python,diff-id=2,diff-type=compliant] +---- +def f(a, b): + if a is None and b is None: do_something() + elif a is not None or b is not None: # Noncompliant + do_something_else() +---- + +==== Compliant solution + +[source,python,diff-id=2,diff-type=noncompliant] ---- +def f(a, b): + if a is None and b is None: + do_something() + else: + do_something_else() +---- + +=== How does this work? + +In the first example, the gratuitous condition `a` is simply `True`. +Hence, the first `if`-statement can be removed, as `do_something()` is always +executed. +Additionally, the second `if`-statement can be simplified since the execution +of `do_something_else()` actually only depends on `b`. + +In the second example, the condition on the `elif`-branch is gratuitous because +it is a logical consequence of a condition that already has been confirmed to +hold: +The condition of the `elif`-branch is only evaluated if the condition on the +`if`-branch evaluates to `False`. +If that condition is `False`, then as a consequence, at least one of `a` or `b` +is not `None`. +This is exactly the circumstance that is formalized by the second condition. +Thus it always evaluates to `True` when checked. +Therefore, the `elif`-branch can be simplified to an `else`-branch without a +condition. include::../see.adoc[] diff --git a/rules/S2589/see.adoc b/rules/S2589/see.adoc index 906bda93c69..70ff278ce20 100644 --- a/rules/S2589/see.adoc +++ b/rules/S2589/see.adoc @@ -1,4 +1,6 @@ == Resources +=== Articles & blog posts + * https://cwe.mitre.org/data/definitions/571[MITRE, CWE-571] - Expression is Always True -* https://cwe.mitre.org/data/definitions/570[MITRE, CWE-570] - Expression is Always False \ No newline at end of file +* https://cwe.mitre.org/data/definitions/570[MITRE, CWE-570] - Expression is Always False