Skip to content

Commit

Permalink
Modify rule S2589: Extend LaYC content for Python (#3173)
Browse files Browse the repository at this point in the history
  • Loading branch information
anton-haubner-sonarsource authored Sep 28, 2023
1 parent 9303db1 commit 193a94f
Show file tree
Hide file tree
Showing 2 changed files with 90 additions and 9 deletions.
95 changes: 87 additions & 8 deletions rules/S2589/python/rule.adoc
Original file line number Diff line number Diff line change
@@ -1,32 +1,111 @@

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
if a: # Noncompliant
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[]

Expand Down
4 changes: 3 additions & 1 deletion rules/S2589/see.adoc
Original file line number Diff line number Diff line change
@@ -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
* https://cwe.mitre.org/data/definitions/570[MITRE, CWE-570] - Expression is Always False

0 comments on commit 193a94f

Please sign in to comment.