`A13-3-1`: Query reports about function overloads at the same location.

[rak3-sh]

Affected rules

• A13-3-1

Description

This query reports two overloaded functions but at the same location in the code base, thereby causing confusion to the user. It may be good to check the location explicitly to avoid such a case. Please note that the implementation already checks if the overloaded function and the candidate function are not the same and also they are not Function Template Instantiation or Specialization. Still, the query results in the same function to be reported as the candidate as well as the overloaded function.

Example

Not able to minimize the problem

[rak3-sh]

Possible Fix

While we can try and understand in greater detail why CodeQL returns two overloaded functions in the same location but at least for this query, it will be pragmatic to check for the difference in location explicitly in the query and removes such unwanted results in FunctionThatContainsForwardingReferenceAsItsArgumentOverloaded.ql,

from
  Candidate c, Function f, Function overload, Function overloaded, string msg,
  string firstMsgSegment
where
  <... snip ...>
  f = c.getAnOverload() and
  <...snip...> 
  // -- Fix start - check location is different
  f.getLocation() != c.getLocation() 
  // -- Fix end 
  <snip> ...
  overloaded = c and
  overload = f
select overload, "Function" + firstMsgSegment + "overloads a $@.", overloaded, msg

[lcartey]

Thanks @rak3-sh!

I believe the problem of multiple overloads at the same location can happen when a single file is compiled multiple times in an introspected build under different contexts (for example, different target platforms).

Fortunately, we already have a library that helps deduplicate in this case - codingstandards.cpp.FunctionEquivalence - and a predicate getAnEquivalentFunction that can find equivalent functions. I think we can exclude the undesirable cases by adding this condition:

...
not f = getAnEquivalentFunction(c) and
...

The reason I would suggest against the getLocation() approach is because it could cause false negatives if a true positive occurred as a macro expansion - then both the function and overload would have the same location in the database, and so would be incorrectly excluded.

[rak3-sh]

Thanks for your insightful comment! It makes sense and it has been changed appropriately in the PR #797