github / codeql-coding-standards

This repository contains CodeQL queries and libraries which support various Coding Standards.
MIT License
110 stars 44 forks source link

`A5-2-6`: Exclude cases with the same binary operator #231

Open lcartey opened 1 year ago

lcartey commented 1 year ago

Affected rules

Description

It's not well defined by the standard, but I think it's reasonable to exclude -> and . when considering whether an operand of a logical operator is binary.

Update: We thought the query was incorrectly reporting -> and . as binary operators. However, the problem was a confusion over this example:

foo->bar() && foo->baz() && foo->bang()

Where the query is reporting that this should be bracketed like so:

(foo->bar() && foo->baz()) && foo->bang()

This is a grey area in the rule itself - technically the title implies we should report this case. However, the rationale states:

Parentheses are required to add clarity in logical expressions making code easier to review versus code based only C++ operator precedence rules.

Which I think argues against asking for the developers to add unnecessary brackets in this case.

Proposed next steps:

Example

foo->bar() && foo->baz() && foo->bang()
lcartey commented 1 year ago

Reviewing the query, the description above is not accurate. The query already only reports BinaryOperations, which does not include FieldAccesses (such as -> and .). I have investigated whether these could be caused by the use of operator->, either independently or in conjunction with templates but cannot reproduce the problem.

lcartey commented 1 year ago

Issue body updated: the problem has been identified as related to (a) confusion caused by the alert message about where the brackets should apply and (b) an over-zealous rule.

nbusser commented 1 month ago

I would like to report a similar over-zealous case when using the same comparison operator:

(a == 0) && (b == 0) // True negative, OK

(a == 0) && (b == 0) && ( c == 0) // Triggers A5-2-6 warning (zealous)

((a == 0) && (b == 0)) && ( c == 0) // Needs to do this to avoid A5-2-6 warning, which alters readability of code

AUTOSAR C++14 is not really clear for this specific case.