Go: Fix missing promoted fields due to name clash

[owen-mc]

When two embedded fields at different depths have the same name (but are not the same type - they are in different packages and happen to have the same name) then we don't treat the second one correctly and we don't promote any of its fields or methods. This turns out to be because of a condition that was added to avoid non-termination on cyclic structs in github/codeql-go#184 which only checked the name of the embedded field. The fix is to also check the type.

While looking into this I noticed that we have two different predicates for calculating field candidates. This PR includes a refactoring to remove the redundancy and make the code easier to understand.

[owen-mc]

Hmm, the tests pass locally for me. I'll have to look into that more.

[owen-mc]

I figured it out. I was using the released CLI, but #17941 changes the extractor and hence the test results, so I needed to build and use the go extractor locally.

This is ready to review now.

[smowton]

Haven't reviewed the tests in detail, but I'm 99% convinced the elimination of getFieldCand vs hasFieldCand is sound. One difference jumps out: getFieldCand uses hasEmbeddedField which doesn't look through a NamedType before using .(PointerType).getBaseType(), getFieldOfEmbedded does. This might make a difference if X is embedded where X is defined type X *Y, in a positive direction. I note hasMethodCand uses this embedded-field predicate as well.

Definitely needs DCA.

[owen-mc]

Note that getFieldCand uses hasEmbeddedField, which is defined using hasFieldCand. So it seems independent, but actually it isn't.

[smowton]

That's true, but for one layer we'll miss named pointer types AFAICT:

    exists(Field f | this.hasFieldCand(_, f, depth, true) |
      tp = f.getType() or
      tp = f.getType().(PointerType).getBaseType()
    )

That means getFieldCand -> hasEmbeddedField will miss the case where there's an embed of type Named which is defined as *MyEmbeddedStruct. For deeper embeddings it then uses hasFieldCand -> getFieldOfEmbedded, so this will only affect depth-1 embeddings.

[owen-mc]

I wrote a test for what you're describing.

type BazPointer *Baz;

type EmbedsBazPointer struct {
	BazPointer
}

It gave me a syntax error: "embedded field type cannot be a pointercompiler" and this link: InvalidPtrEmbed. The relevant part of the spec says:

An embedded field must be specified as a type name T or as a pointer to a non-interface type name *T, and T itself may not be a pointer type. The unqualified type name acts as the field name.

So I think it's okay that the new code-path doesn't exactly do what the old one did, because the missing case is for invalid code.

[smowton]

OK great. That actually suggests then to simplify getFieldOfEmbedded since that does a getUnderlyingType jump prior to checking for a pointer type that in fact can't happen.

[owen-mc]

I ran DCA before the simplification and it was fine. I've started another one. Clearly that code had been inefficient before, because it had a pragma. Hopefully with the simplification which means it can use lookThroughPointerType it will be optimized well without any guidance this time.

[owen-mc]

The second DCA seems to show a 2% speed up on the two biggest dbs. I should probably run QA in case there are any pathological cases.

[owen-mc]

I messed up the QA run by using a badly chosen nightly base, so the results are dominated by the results for #17494. However, I can see that there aren't any alert changes in queries which aren't affected by that PR, and there aren't any noticeable slowdowns. So I am okay with merging this now.