Bugfix: avoid spurious tautology pruning when dealing with certain recursive messages

[rodaine]

When protovalidate-go builds up evaluators for a message, it first adds the pointer to the evaluator cache so any fields that recursively include that message will use that instead of attempting to rebuild the message infinitely until a stack overflow.

Also as part of this build process, any sub-evaluators that would tautologically validate (i.e., always be valid) are pruned to optimize the resulting evaluator for the message. Generally speaking, if an evaluator has no sub-evaluators or CEL expressions attached to it, then it's considered a tautology.

Individually, these features work well, but together they could collide causing validations to be lost for certain recursive message structures. For example, with the following message graph (arrows meaning "has a field of type"):

  graph TD;
      M1-->M2;
      M2-->M3;
      M3-->M2;
      M1-->M3;

Loading

1. Suppose in this case, the only constraint is defined on M2.
2. M1 starts to build, and as part of that, it starts to build M2.
3. This continues to M3.
4. M3 recursively depends on M2, and ends up loading the still incomplete pointer to M2's evaluator from the cache.
5. Before adding it to M3's list of sub-evaluators, we check if it's a tautology.
6. Depending on field ordering, M2's evaluator might have no sub-evaluators yet for its constraint, resulting in a false positive.
7. This means M3 would not include the M2 sub-evaluator.
8. Bubbling up back to M2, it would also check if M3's evaluator is a tautology, and since M2's was excluded, M3 is also a tautology false positive, and won't be included in M2's sub-evaluators.
9. Then, however, M2 will pick up its actual constraints, resulting in M1 not detecting a tautology.
10. Finally, M1 pulls M3's evaluator from the cache, sees that it is (wrongly) a tautology, and does not include it in its sub-evaluators.

---

Originally, I attempted to make the tautology detection check if the message's evaluator was "ready," but this resulted in infinite recursion when checking tautology between the messages. For now, instead, we treat all message evaluators as never a tautology. This solves the underlying issue, at the cost of nominally less optimal evaluators. This is certainly acceptable in that any other solution will likely make the evaluator builder more complex than it already is.

---

Longer term solutions to explore:

• Track recursion and short-circuit when detected. Similar to this solution, but would only occur in recursive scenarios.
• Do tautological pruning as a second pass after the evaluator has been completely constructed. Less efficient than doing it as we go, but will ensure that everything is built prior to the check.
• Leave it as-is. The impact on memory and CPU is minimal.

Fixes #141

[github-actions]

The latest Buf updates on your PR. Results from workflow Buf / validate-protos (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	✅ passed	✅ passed	✅ passed	Aug 29, 2024, 12:14 AM

[daemonl]

Can confirm this branch fixes both my initial case and the example case! Thanks heaps!