Fix code generation bugs in refined reference/attribute handling - #144
Closed
ChrisH07 wants to merge 9 commits into
Closed
Fix code generation bugs in refined reference/attribute handling#144ChrisH07 wants to merge 9 commits into
ChrisH07 wants to merge 9 commits into
Conversation
added 7 commits
July 22, 2026 11:03
AreConflicting compared only member name and CLR type, so two properties implementing different interfaces with the same name (e.g. a refined reference redeclared via explicit interface implementation) were treated as a naming collision. Since properties don't support merging, the fallback silently renamed one of them, breaking the interface implementation. Now compares PrivateImplementationType too.
NMF.Models.Meta is imported into every generated file and declares its own class named Type, so attributes mapped to System.Type generated an unqualified "Type" reference that was ambiguous between the two.
Feature2Proxy named each generated proxy nested class only from the feature's short name. A reference that refines/redefines a base feature keeps the same name, so its proxy collided with the base feature's own proxy. The merge-conflict fallback then renamed one of the two shared, memoized CodeTypeDeclaration instances in place, which could corrupt unrelated classes that referenced the same instance and made output non-deterministic across runs. Proxy names are now qualified by declaring type so the collision can't occur; Class2Type's matching string-based type reference is updated to stay in sync.
Reflects the previous commit's fix that qualifies generated proxy class names with their declaring type.
SetFeature cast "this" to the type of the refined feature's value instead of the type that declares it, so setting a base reference or attribute by name through the generic feature API cast to the wrong interface and could no longer see the property being set.
…ction
The collection class generated for a refined multi-valued reference or
attribute was named "{scope}{featureName}Collection", which collided
with the unrelated nested "{scope}ChildrenCollection" class the code
generator always creates for a class's containment children whenever a
feature happened to be named "Children". C# resolved the unqualified
name to the wrong (nested) class, producing a container implementation
that didn't implement the interface its property declared.
None of the existing code generation tests exercise Refines: it requires a class re-declaring a same-named feature from an interface supertype. Adds a minimal model with a single-valued reference and a containment collection reference both refined from an interface base, matching the shape that triggered several recent bugs. Verified this test fails (merge-conflict exceptions and/or compile errors) when any of those fixes are individually reverted.
⚡ Risk Assessment —
|
| Files | Summary |
|---|---|
Explicit Interface Implementation Conflict ResolutionTransformations/CodeGen/ClassGenerator.cs |
Adds SamePrivateImplementationType check to AreConflicting method for methods, properties, and events to prevent false conflicts between explicit interface implementations targeting different interfaces |
Proxy Naming Convention FixTransformations/Models.MetaTransformation/Meta/Feature2Proxy.csTransformations/Models.MetaTransformation/Meta/Class2Type.csTransformations/Tests/CodeGenerationTests/References/railway.csTransformations/Tests/CodeGenerationTests/References/Families.csTransformations/Tests/CodeGenerationTests/References/Relational.csTransformations/Tests/CodeGenerationTests/References/OperationTest.csTransformations/Tests/CodeGenerationTests/References/architectureCRA.csTransformations/Tests/CodeGenerationTests/References/DefaultValueTest.csTransformations/Tests/CodeGenerationTests/References/FromSchemaEcore.csTransformations/Tests/CodeGenerationTests/References/NameClashes.csTransformations/Tests/CodeGenerationTests/References/Persons.cs |
Changes proxy class naming from {FeatureName}Proxy to {DeclaringTypeName}{FeatureName}Proxy to avoid naming collisions when refining/redefining base features. Updates all generated test code to use new naming convention. |
Type Reference Full Name FixTransformations/Models.MetaTransformation/Meta/Meta2ClassesTransformation.cs |
Ensures System.Type always uses FullName to avoid ambiguity with NMF.Models.Meta namespace which also declares a Type class |
Refined Collection Naming FixTransformations/Models.MetaTransformation/Meta/RefinedAttributeCollectionClassGenerator.cs, RefinedReferenceCollectionClassGenerator.cs |
Adds 'Refined' prefix to collection class names to avoid collision with Class2Children nested types when attribute/reference is named 'Children' |
SetFeature Refined Feature Dispatching FixTransformations/Models.MetaTransformation/Meta/Class2Type.cs |
Fixes SetFeature to skip refined references/attributes in regular AddReferencesOfClass/AddAttributesOfClass processing to avoid shadowing the refined dispatching case. Also fixes type resolution to use f.DeclaringType instead of f.Type for proper type casting. |
Test Infrastructure UpdatesTransformations/Tests/CodeGenerationTests/ModelTests.csTransformations/Tests/CodeGenerationTests/CodeGenerationTests.csprojTransformations/Tests/CodeGenerationTests/Refines.ecoreTransformations/Tests/CodeGenerationTests/References/Refines.cs |
Adds new test case for Refines.ecore metamodel and includes generated Refines.cs in project |
CodeGen History UpdateTransformations/CodeGen.history |
Adds patch entry documenting the refined references and attributes code generation fix |
Sequence Diagram
sequenceDiagram
participant M as Metamodel (Ecore)
participant T as Meta2ClassesTransformation
participant F2P as Feature2Proxy
participant C2T as Class2Type
participant CG as ClassGenerator
participant G as Generated Code
M->>T: Process Refines.ecore
T->>F2P: Transform ITypedElement to proxy
F2P->>F2P: Generate qualified name (DeclaringType + FeatureName + Proxy)
T->>C2T: Add refined references/attributes
C2T->>C2T: Use f.DeclaringType for type resolution
C2T->>C2T: Skip refined features in regular SetFeature processing
C2T->>CG: Add members to type
CG->>CG: Check AreConflicting with PrivateImplementationType
CG-->>T: No conflict (different interfaces)
T-->>G: Generate valid code
Dig Deeper With Commands
/review <file-path> <function-optional>/chat <file-path> "<question>"/roast <file-path>
Runs only when explicitly triggered.
![mergemonkeyhq[bot]](https://avatars.githubusercontent.com/in/2472657?s=60&v=4){kind=small}
Comment on lines
1235
to
+1239
| { | ||
| if (feature.UpperBound == 1) | ||
| { | ||
| var propTypeRef = new CodeTypeReference(feature.Name.ToPascalCase() + "Proxy"); | ||
| // Must match the name Feature2Proxy assigns to the generated proxy nested type. | ||
| var propTypeRef = new CodeTypeReference((feature.Parent as IType).Name.ToPascalCase() + feature.Name.ToPascalCase() + "Proxy"); |
There was a problem hiding this comment.
Refined feature generation still emits colliding SetFeature keys
The proxy-name change is correct, but the refined setter path still generates branches keyed only by the original feature name. In the new Refines output that creates two feature == "ITEM" cases in Concrete.SetFeature, so the first branch returns and the refined-base assignment is never reachable.
When emitting refined SetFeature cases, give the refined slot a distinct generated key or merge both assignments into a single branch when they intentionally share the same external feature name.
Was this helpful?
- 👍 Yes
- 👎 No
Actionable Comments Posted: 1🧾 Coverage Summary✔️ Covered (19 files) |
added 2 commits
July 22, 2026 13:22
A reference or attribute that refines a base feature shares its external name, so SetFeature generated two branches keyed on the same upper-cased name: one for the feature's own declaration and one for the refined slot. Since the first always returns, the refined branch - the one that correctly dispatches to whichever implementation applies - was unreachable. Skip the feature's own branch when it refines another feature, since the refined branch already covers it. Found by MergeMonkey review.
Contributor
Author
|
Closing in favor of a new PR from a slash-free branch name — this branch's slash breaks the EtiCat prerelease-version-string check in CI. See the replacement PR for the same changes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes several bugs in
Meta2ClassesTransformation's code generation for refined ("Refines") references and attributes, found while generating code for a metamodel that combines multiple base metamodels through inheritance.ClassGenerator.AreConflicting) — the conflict detector ignoredPrivateImplementationType, so two properties implementing different interfaces with the same name (e.g. a refined reference's explicit interface implementation) were mistaken for a naming collision and one got silently renamed, breaking the interface implementation.Feature2Proxy) — generated proxy nested classes were named only from the feature's short name, so a refined reference collided with its base's proxy. The unsafe rename fallback then mutated a shared/memoized object, making output non-deterministic across runs. Proxy names are now qualified by declaring type.SetFeature(Class2Type) — castingthisused the refined feature's value type instead of its declaring type, so setting a base reference/attribute by name cast to the wrong interface.RefinedReferenceCollectionClassGenerator/RefinedAttributeCollectionClassGenerator) — a refined reference/attribute named "Children" generated a collection class with the same name as the framework's own children-aggregation class, so C# resolved to the wrong one.Also included, found in the same investigation but unrelated to Refines:
System.Typereference (Meta2ClassesTransformation.CreateReferenceForMappedType) — attributes typed with NMF's built-inSystemTypeprimitive generated an unqualifiedTypereference that collided withNMF.Models.Meta.Type, which is imported into every generated file by default. This is a general bug independent of the others; it just surfaced in the same test model.Testing:
CodeGenerationTests,Transformations.Tests,EcoreInterop.Tests), with golden-reference snapshots updated to reflect the intentional proxy-naming change.Refines.ecore/RefinesModelGeneratedSuccessfully). The new model has a class refining both a single-valued reference and a "Children"-named containment collection from an interface base — the same shape that triggered all fourRefinesbugs above. Verified this test fails (with the original merge-conflict exceptions and/or compile errors) when any one of the four fixes is individually reverted, confirming it's a meaningful regression guard.Summary by MergeMonkey