fix: use raw enum values for cross-enum conflict detection - #2390
mromaszewicz merged 9 commits into
Conversation
ddaed3d to
00d47d2
Compare
Greptile SummaryThis PR makes enum conflict detection use stable enum value names. It changes:
Confidence Score: 5/5This looks safe to merge.
|
| Filename | Overview |
|---|---|
| pkg/codegen/codegen.go | Updates enum conflict detection to iterate to a stable set of prefixed enum constants. |
| pkg/codegen/configuration.go | Adds the compatibility flag for disabling the new enum value conflict resolution. |
| configuration-schema.json | Exposes the new compatibility flag in the published configuration schema. |
| pkg/codegen/codegen_test.go | Adds tests for order-independent enum conflict detection. |
Reviews (3): Last reviewed commit: "Simplify enum value de-dup" | Re-trigger Greptile
|
Summoning @jamietanna for review |
GetValues() returns prefixed identifier names for enums already marked as conflicting. Using it in the cross-enum comparison loop makes the result order-dependent: an enum that was prefixed in an earlier iteration produces keys like FooStateReady, which no longer match the unprefixed key Ready in a later enum, so the later enum misses the conflict and stays unprefixed. Fix by comparing Schema.EnumValues keys directly, which are always the raw (unprefixed) Go-safe identifiers regardless of PrefixTypeName state.
7fa6bd9 to
930146a
Compare
|
CI failed because your change affects existing generated code. I pulled your PR locally and I'm investigating, but we might need to feature-flag this to avoid breaking existing users. |
|
I had Claude dig into this locally: Why the files changeThis PR changes conflict detection from comparing generated constant names (
|
|
@natalie-o-perret - This code is all extremely sensitive. We can make new behavior default-on, but there definitely needs to be a flag to revert to current behavior, since I don't see a way of fixing this properly without breaking some old boilerplate. |
1fb4e36 to
be163f0
Compare
Thanks for the thorough write-up. I think you may have pulled before the second force push. The current commit uses a 2-stage algorithm that handles both conflict classes.
Agreed, that's exactly the bug this PR is fixing.
This is handled by Stage 2. After Stage 1 marks Enum1 as prefixed (it shares raw values with Enum2), Stage 2 runs an iterative
Fair point, I've added a |
|
@greptileai re-review. |
| // already-applied prefixes, so enums processed later may miss conflicts | ||
| // with enums that were prefixed in an earlier iteration. Preserved here | ||
| // as an escape hatch for users who need to keep existing generated output. | ||
| for i := range enums { |
There was a problem hiding this comment.
If I understand this right, the next loop completely supersedes this one, so this one doesn't even need to run.
There was a problem hiding this comment.
Not quite, Stage 2 alone misses CState in the AState/BState/CState case.
Once Stage 2's first pass prefixes AState because it shares "running" with BState, AState.GetValues() returns {AStateRunning, AStateMigrating}.
When it then checks AState vs CState, "migrating" in CState no longer matches any key in AState's now-prefixed values, so CState is left unprefixed.
I verified this by removing Stage 1 and running the tests, both TestEnumConflictDetectionOrderIndependent and TestEnumConflictDetectionBothOrders fail with Migrating CState = "migrating" in the output.
|
Sorry for the delay, I am revisting this now. We can't change the meaning of old flags if we can help it. I will push a change to default-on your change, but allow disabling it via |
Gate enum value conflict resolution behind a new `disable-enum-value-conflict-resolution` option (default on) instead of overloading `old-enum-conflicts`, and revert the unrelated configuration comment churn. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The conflict-resolution path had two loops: a "Stage 1" pass comparing raw enum values, followed by a "Stage 2" fixed-point loop comparing effective (possibly prefixed) constant names. Stage 1 was redundant. For an unprefixed enum, GetValues() returns the raw EnumValues, so Stage 2's first iteration already performs exactly the raw comparison Stage 1 did. The two passes only diverge when a single GenerateEnums call mixes already-prefixed and unprefixed enums, which never happens at either call site: server-URL enums are all force-prefixed (and generated in isolation), the main call contains none of them, and always-prefix-enum-values is global. So dropping Stage 1 leaves the generated output unchanged. Collapse to the single fixed-point loop, which is genuinely required: a prefixed name (e.g. "Enum1One") can collide with another enum's raw value, so resolution must iterate until stable. Also operate on the slice elements directly instead of the copy/write-back dance, and factor the overlap test into enumsShareConstantName (which now builds each side's value map once per pair rather than rebuilding the second one per key). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…gen#2390) * fix: use raw enum values for cross-enum conflict detection GetValues() returns prefixed identifier names for enums already marked as conflicting. Using it in the cross-enum comparison loop makes the result order-dependent: an enum that was prefixed in an earlier iteration produces keys like FooStateReady, which no longer match the unprefixed key Ready in a later enum, so the later enum misses the conflict and stays unprefixed. Fix by comparing Schema.EnumValues keys directly, which are always the raw (unprefixed) Go-safe identifiers regardless of PrefixTypeName state. * test: add order-independence test for both schema orderings * feat: add old-enum-conflict-detection compatibility flag * fix: reuse old-enum-conflicts flag for legacy conflict detection path * clean up configuration Gate enum value conflict resolution behind a new `disable-enum-value-conflict-resolution` option (default on) instead of overloading `old-enum-conflicts`, and revert the unrelated configuration comment churn. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Simplify enum value de-dup The conflict-resolution path had two loops: a "Stage 1" pass comparing raw enum values, followed by a "Stage 2" fixed-point loop comparing effective (possibly prefixed) constant names. Stage 1 was redundant. For an unprefixed enum, GetValues() returns the raw EnumValues, so Stage 2's first iteration already performs exactly the raw comparison Stage 1 did. The two passes only diverge when a single GenerateEnums call mixes already-prefixed and unprefixed enums, which never happens at either call site: server-URL enums are all force-prefixed (and generated in isolation), the main call contains none of them, and always-prefix-enum-values is global. So dropping Stage 1 leaves the generated output unchanged. Collapse to the single fixed-point loop, which is genuinely required: a prefixed name (e.g. "Enum1One") can collide with another enum's raw value, so resolution must iterate until stable. Also operate on the slice elements directly instead of the copy/write-back dance, and factor the overlap test into enumsShareConstantName (which now builds each side's value map once per pair rather than rebuilding the second one per key). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Marcin Romaszewicz <marcinr@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fixes #2391.
Cross-enum conflict detection uses
e1.GetValues()to check for shared values, butGetValues()returns prefixed names for enums already marked in an earlier loop iteration. Those prefixed names don't match the raw values of later enums, so some conflicts are silently missed, and whether an enum gets prefixed depends on iteration order rather than actual value overlap.Fix: compare
e1.Schema.EnumValueskeys instead. These are always the unprefixed identifiers, regardless of whether the enum has been marked yet.Regression test added (
TestEnumConflictDetectionOrderIndependent): three string enums where A↔B and A↔C both conflict, but the A↔C conflict was previously missed once A was already prefixed.Discovered via a downstream build failure after an upstream schema change removed a schema that had been the only direct conflict anchor for one of the enums.