The .expected output contains three identical rows for the same location (test.cpp:42:16). This indicates the query is emitting duplicates; tests should generally not codify duplicate results. Once the query is de-duplicated, update this expected file accordingly.

The expected results for this rule report the primary location on INT i; (line 4), but the test marks extern int i; (line 5) as NON_COMPLIANT. Please align the NON_COMPLIANT annotation(s) with the location(s) the query reports (either move/add the annotation to line 4, or adjust the query to report line 5 as the primary location).

I'm usually not a fan of getAChild*(), because it is typically a bigger hammer than we want. For instance, this will return FPs for return ref ? 1 : 2, or return ref_to_vector.get_size(), etc.

Can we use localFlow instead?

I have now opted to direct explicit references only... I do agree localFlow within the statement might make the most sense, but it would maybe be overkill? let me know though if that seems like it would miss reasonable things and I will add it!

This isn't going to do exactly what we probably want, because we have to worry again about pointers and pointees.

Returning a pointer by value is allowed, and dereferencing that pointer should also be allowed because the result could be any object -- not guaranteed to be a subobject of this. And returning an address or reference to a field that is a pointer should be flagged as well.

class C {
  int i;
  int *ip;
  int **ip2;

  int *f() {
    return &i; // non compliant
    return ip; // compliant -- copies pointer
    return *ip2; // compliant -- reads pointer
  }

  int **f2() {
    return &ip; // non compliant
    return ip2; // compliant -- copies pointer
  }

  int &f3() {
    return i; // non compliant
    return *ip; // compliant -- reads pointer
    return **ip2; // compliant -- reads pointer
  }

  int *&f4() {
    return &p; // won't compile
    return ip; // non compliant
    return *ip2; // compliant -- reads pointer
  }

  int **&f5() {
    return &ip; // won't compile
    return ip; // non compliant
  }
}

added this test, as well as incorporated feedback from overall review comment , some FPs remain, I think I need a second set of eyes on my expansion of cppobjects at this point

Hmm....I think a field of reference type should still be a subobject of itself. So this should be outside of the above condition.

It looks like using getAChild*() finds too many results here.

For example:

my_var                         // 1
  .get_a_field_of_non_ref_type // 2
  .get_a_field_of_ref_type     // 3

Here, 3 is not a subobject of 1. However, 2 is a child of 3, and 2 is a field access of non-ref type, so it satisfies f. So this will give the wrong answer here.

I think the correct implementation is simpler:

result = e
or
result.(DotFieldAccess).getQualifier = getASubobjectAccessOf(e) and
not result.(DotFieldAccess).getTarget().getUnderlyingType() instanceof ReferenceType
or
....

Might be clearer to write with a more detailed comment, which I'm happy to help fill in!

Based on the name, I'd say this predicate takes an expression e with pointer type pointer, and finds subobjects of the thing that e points to. Doing so is just a matter of dereferencing e, and then potentially getting subobject accesses of that result.

The expression e may be dereferenced either with *e, getting the pointee itself, or with e->x, immediately getting a subobject of the pointee. Any subobject access of these expressions (e.g. (*e).x, or e->x.y[0].b... are subobject accesses of the pointee of expression e.

This predicate is especially useful for finding subobject accesses of *this.

I may have put the wrong code here in my earlier review, and if so I'm sorry!

We don't want to find &x, we want to find *x, which is a PointerDereferenceExpr

Here we can say

r.getAChild() = getASubobjectAccessOf(t).

That will handle: return *this, return this->x, return (*this)[4], etc.

We'll need to say or r.getAChild() = t to handle return this.

Mostly right but subtle difference!

Given e1->e2, we want the result to be e2 when the parameter is e1. (or a subobject access of e2).

In this case we don't want to use getASubobjectAccessOfPointee on the access.

It looks like this case is handling:

return my_member;
return my_member.subobject;
return my_member.subobject[n];
// etc

So in this case, we can say e = getASubobjectAccessOf(access). But we should also handle return &my_member too.

So, in these two branches here, we don't necessarily care if e has a pointer type or e has a reference type so long as e = getASubobjectAccessOf(access).

The two cases that we need to handle, though, are:

• pointers to subobjects, and
• references to subobjects

So if e is a subobject, then:

• returning the address of e is not allowed (as in return &...)
• returning e directly is not allowed if the method returns by reference (as in int &get_ref() { return e; })

I do have to apologize though because there's one more caveat that makes this confusing 😅. I think it's the last thing. We have to not only check that e = getASubobjectAccessOf(access), but we also have to check if access is not a reference type. That's just because an access of reference type isn't a subobject in the first place, if that makes sense.