Declarative property lookup dispatches on type instead of using overriding 🤖

[lwrage]

Summary

PropertyImpl.getPropertyValueFromDeclarativeModel selects the declarative-model property lookup with an instanceof chain over Subcomponent, FeatureGroup, Feature, and PortConnection, casting to the *Impl class in each branch to reach a method that is not declared on any interface. This hand-rolls virtual dispatch in the caller, and it does so in PropertyImpl, which owns none of the types being dispatched over.

Three consequences:

• The dispatch is fragile in the direction that matters. The chain is correct today only because its branch order happens to place FeatureGroup before Feature. Adding a declarative type, or adding an override on a subtype, requires editing PropertyImpl rather than the type that changed — and forgetting to do so fails silently by falling into the else branch.
• The *Impl casts bypass the interface layer. getPropertyValue(Property, PropertyAcc, Classifier, boolean) is declared only on SubcomponentImpl and FeatureImpl, and getPropertyValue(Property, PropertyAcc) only on PortConnectionImpl. Nothing in the org.osate.aadl2 interfaces describes this operation, so the caller must know the implementation classes by name.
• The all parameter is dead. PropertyImpl is the only caller of these overloads in the repository, and it passes false at every site. Every if (!all) { return; } inside SubcomponentImpl.getPropertyValue, FeatureImpl.getPropertyValue, and FeatureImpl.getPropertyValueHelper is therefore unconditionally taken, and the all argument threaded into the downstream getPropertyValueInternal(prop, pas, true, all) calls is always false.

Reproduction

No observable failure; this is a structural issue. The claims above are established by search:

• getPropertyValue(this, pas, cl, false) and getPropertyValue(this, pas) appear only in PropertyImpl.getPropertyValueFromDeclarativeModel. There is no other caller of the four-argument overloads, and false is the only value ever passed for all.
• Every NamedElement implementer in the repository routes through NamedElementImpl, including the instance metamodel by way of InstanceObjectImpl extends NamedElementImpl. NamedElementImpl is the only class that implements NamedElement without inheriting an implementation.

Expected behavior

A behavior-preserving refactor that moves the dispatch onto the types being dispatched over. Proposed shape:

Declare on the NamedElement interface:

void getPropertyValueForInstance(Property property, PropertyAcc pas, Classifier instantiatedClassifier);

This is additive and precedented — NamedElement already hand-declares getPropertyValueInternal alongside its generated members — and because every implementer inherits from NamedElementImpl, supplying the base body there cannot break any implementer.

• NamedElementImpl — base body delegating to getPropertyValueInternal(property, pas, true), which is what the current else branch does.
• SubcomponentImpl, FeatureImpl, PortConnectionImpl — overrides carrying the existing bodies.
• PropertyImpl.getPropertyValueFromDeclarativeModel — collapses to a guard clause plus a single call, with no casts.

Declare the new method without the all parameter. That captures the simplification as new API rather than as a signature change to existing API, and reduces the question of what to do with the old overloads to a single decision:

• Preferred: delete the now-unreferenced getPropertyValue overloads on SubcomponentImpl and FeatureImpl, FeatureImpl.getPropertyValueHelper, and PortConnectionImpl.getPropertyValue, along with the dead if (!all) branches inside them.
• Conservative: retain them as @Deprecated delegates that ignore all, preserving binary compatibility and leaving the dead branches in place.

org.osate.aadl2.impl is exported from the bundle with no x-internal or x-friends, so the preferred option is a binary-incompatible change against the 2.18.0 API baseline. The tycho-p2-extras-plugin baseline comparison is currently commented out, so nothing in the build will flag the choice — it needs a deliberate decision rather than a green build.

Since there is no behavior change, this carries no regression test of its own and rests on the existing property-lookup suite; the branch should record a green baseline for that suite before and after.

Relevant code

• core/org.osate.aadl2/src/org/osate/aadl2/impl/PropertyImpl.java, getPropertyValueFromDeclarativeModel — the instanceof chain
• core/org.osate.aadl2/src/org/osate/aadl2/NamedElement.java — where the new method would be declared, next to the existing hand-declared getPropertyValueInternal
• core/org.osate.aadl2/src/org/osate/aadl2/impl/NamedElementImpl.java, getPropertyValueInternal
• core/org.osate.aadl2/src/org/osate/aadl2/impl/SubcomponentImpl.java, getPropertyValue
• core/org.osate.aadl2/src/org/osate/aadl2/impl/FeatureImpl.java, getPropertyValue, getPropertyValueHelper
• core/org.osate.aadl2/src/org/osate/aadl2/impl/PortConnectionImpl.java, getPropertyValue
• core/org.osate.aadl2/src/org/osate/aadl2/instance/impl/InstanceObjectImpl.java — confirms implementers inherit from NamedElementImpl

Dependencies

Depends on #3107, which removes the redundant FeatureGroup branch. Doing that first means one fewer branch to collapse and one fewer override to consider here.

Related

• Looking up instance properties in the declarative model #875 asks whether instance property lookup should consult the declarative model at all. This issue is deliberately narrower: it restructures the dispatch without taking a position on that question, and it does not make the eventual answer harder to act on.
• Follows the property-lookup work in Speed up model instantiation by eliminating redundant recomputation 🤖 #3095 and Modernize the property caching code in model instantiation 🤖 #3101.