Fix field lookup in cyclic structs

[max-schaefer]

Our analysis wasn't terminating on https://github.com/hashicorp/nomad due to it trying to look up a field in a cyclic struct. (I'm quite surprised this hasn't happened earlier.)

The problem is in the part of our library that attempts to model the rules for resolving field or method references x.f in the presence of embedded fields. While you might think that it is an error to embed two or more like-named fields into the same struct, that isn't actually the case: it's fine to do that, as long as one of the fields has a smaller depth than the others, where the depth of a field is "the [minimum] number of embedded fields traversed to reach f".

Trying to define this directly using recursion in QL is tricky, since (as far as the compiler can tell) it involves non-monotonic recursion through an aggregate. So instead we first collect a set of candidate fields with their depths, and then choose the shallowest. But when I wrote that code I didn't account for cyclic embeddings, which would make the predicate for collecting candidates infinite.

We now short-circuit by only looking through embedded fields if there isn't a field of the same name declared directly in the struct. I haven't been able to convince myself that this is sufficient, but it does fix the problem on nomad, and I wasn't immediately able to construct an example where it still fails.

[max-schaefer]

Still evaluating, so marking as draft for the time being.

[max-schaefer]

Evaluation (internal link) was unremarkable.

@sauyon, if you can find the time it would be great if you could take a look at this over the weekend so we can get it into next week's dist upgrade.

[sauyon]

Sorry, it took me some time to work out exactly what was going wrong.

Having not looked at the nomad code at all, I'm actually reasonably surprised that people would want cyclic embedded types at all; that seems like a recipe for confusion. Perhaps it's a type system hack of some kind.

I think you're right that this should fix the issue, since if there is a cycle the recursion must eventually hit a type that has the field in question, which should then stop the recursion.

I assume if the field does not actually exist, no tuples are generated and so the fixed point converges after one step? (According to my somewhat shaky understanding of the semantics of recursion)

[max-schaefer]

I'm actually reasonably surprised that people would want cyclic embedded types at all

Yeah, I think you're right. It doesn't sound terribly useful.

I assume if the field does not actually exist, no tuples are generated and so the fixed point converges after one step?

Kind of. Fixpoints are computed iteratively and bottom-up, so in the first iteration we'll get all fields declared directly in the type, with depth 0, and in iteration n+1 we'll get those, plus all fields of depth declared in an embedded type with their depths increased by one. In the presence of cyclic types, we would keep computed greater and greater depths for the same field, so we now short-circuit as soon as there is a field of depth 0 of the same name, since in that case we'll definitely choose it over the embedded fields anyway.

[max-schaefer]

The cyclic type is Server: its embedded fields rpcHandler, planner, and nodeHeartbeater each in turn embed Server (through pointers).

[sennap]

hey folks 👋 I was wondering if I could confirm that this got included in with the most recent codeql bundle?

[max-schaefer]

I can confirm that it is deployed on LGTM.com. @robertbrignull, I assume this means it's also part of the most recent bundle?

[robertbrignull]

The codeql-go commit currently used on code scanning is d8374adbdef2470e3c000c4c1360feada2b91fb1 and that does include this PR.