Fix code generation bugs in refined reference/attribute handling - #145
Fix code generation bugs in refined reference/attribute handling#145ChrisH07 wants to merge 9 commits into
Conversation
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.
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.
⚡ Risk Assessment —
|
| Files | Summary |
|---|---|
Explicit Interface Implementation Conflict DetectionTransformations/CodeGen/ClassGenerator.cs |
Adds SamePrivateImplementationType helper and extends AreConflicting to check private implementation types for methods, properties, and events, fixing false conflicts with explicit interface implementations |
Proxy Naming Convention FixTransformations/Models.MetaTransformation/Meta/Feature2Proxy.cs, Class2Type.cs |
Changes proxy class naming from PropertyNameProxy to TypeNamePropertyNameProxy to avoid collisions when refining base features, and updates SetFeature to skip refined features in regular handling |
Refined Collection NamingTransformations/Models.MetaTransformation/Meta/RefinedReferenceCollectionClassGenerator.cs, RefinedAttributeCollectionClassGenerator.cs |
Adds 'Refined' suffix to collection class names to avoid collision with Class2Children nested types when attributes/references are named 'Children' |
System.Type QualificationTransformations/Models.MetaTransformation/Meta/Meta2ClassesTransformation.cs |
Fully qualifies System.Type with .FullName to avoid ambiguity with NMF.Models.Meta.Type namespace class |
Test InfrastructureTransformations/Tests/CodeGenerationTests/ModelTests.csTransformations/Tests/CodeGenerationTests/CodeGenerationTests.csprojTransformations/Tests/CodeGenerationTests/Refines.ecoreTransformations/Tests/CodeGenerationTests/References/Refines.cs |
Adds new test case and test files for refined reference/attribute code generation validation |
Generated Test Code UpdatesTransformations/Tests/CodeGenerationTests/References/railway.cs, Families.cs, Relational.cs, OperationTest.cs, architectureCRA.cs, DefaultValueTest.cs, FromSchemaEcore.cs, NameClashes.cs, Persons.cs |
Updates proxy class names to new naming convention (e.g., LengthProxy → SegmentLengthProxy) |
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: Transform MetaModel
T->>F2P: Generate proxy for each feature
F2P->>F2P: Use declaring type in proxy name (TypeNamePropertyNameProxy)
F2P-->>T: Proxy type generated
T->>C2T: Generate class members
C2T->>C2T: Skip refined features in regular AddReferencesOfClass
C2T->>C2T: Use same naming in AddToExpressionForFeature
C2T->>CG: Check for conflicting members
CG->>CG: Compare PrivateImplementationType for methods/properties/events
CG-->>C2T: No conflict (different interfaces)
C2T-->>T: Class members generated
T-->>G: Complete generated code
Dig Deeper With Commands
/review <file-path> <function-optional>/chat <file-path> "<question>"/roast <file-path>
Runs only when explicitly triggered.
|
There is a related issue in Reference2Property that I did not push with this PR. Running Ecore2Code of a file that used Refines to covariantly narrow a containment reference's element type while restating that reference's Opposite crashed with NotImplementedException in Reference2Property.GenerateOppositeAttributes: NMF/Transformations/Models.MetaTransformation/Meta/Reference2Property.cs Lines 610 to 618 in faecef7 That method only knows how to emit [XmlOpposite] when opposite.ReferenceType exactly equaled input.DeclaringType. Since [XmlOpposite] is actually resolved at runtime purely by feature name, and Refines always preserves the name, the check was unnecessary. The fix was to remove the check and always emit [XmlOpposite(input.Opposite.Name)] when an opposite exists. But this leaves the possibility of the Opposite being silently wiped out. I didn't see any specific mechanism of reporting this case back to Ecore2Code so I locally added a Console.WriteLine statement. If you have a preference on how to handle this situation I can make the change, update the tests, and update this PR. |
|
I am currently on holidays. Is the fix urgent? Otherwise, I will review the PR in August when I get back |
No rush at all. I am not blocked. |
Replaces #144, which was closed only because its branch name (
fix/refines-code-generation) contains a slash that breaks the EtiCat prerelease-version-string check in CI. Same commits, no other changes.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.SetFeaturecase for refined features (Class2Type) — a reference/attribute that refines a base feature shares its external name, soSetFeaturegenerated two branches keyed on the same name; the first always returned, so the refined (correctly dispatching) branch was never reached. Found by MergeMonkey review on Fix code generation bugs in refined reference/attribute handling #144.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): none of the existing sample models inCodeGenerationTestsactually exerciseRefines(it requires a class to redeclare a same-named feature inherited from an interface supertype, and none of the existing.ecoremodels declare multipleeSuperTypes), so these bugs had no coverage at all. 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 theRefinesbugs above. Verified this test fails (with the original merge-conflict exceptions and/or compile errors) when any one of the fixes is individually reverted, confirming it's a meaningful regression guard.Summary by MergeMonkey