Bug 418716

Comment 1 Ed Willink 2013-10-05 17:56:57 EDT

This is a bug that dates right back to the introduction of setting delegates. Since OCL is side effect free it was assumed that the required functionality was just get().

Of course for a changeable variable with a delegate, the first get() is an init of a cached value which thereafter supports conventional get/set. Unfortunately the EMF templates could implement this directly for non-volatile variables with a delegate, but they don't.

The UnsupportedOperationException comes from the missing OCLSettingDelegate.set override that could be updating the cache.

Comment 2 Ed Willink 2013-10-08 13:04:03 EDT

Hi Ed

Arguably there is a missing SettingDelegate use case (current behaviour is UnsupportedOperationException).

For a non-volatile, changeable feature, with a delegated initializer:

non-volatile => there is a private feature value cache
changeable => there is a setter and a getter
delegated initializer => construction/first-get computes the initializer

So I could envisage the pseudo-code:

private T xxx = 'unset';

getXXX() {
    if (xxx.eIsUnset()) {
        xxx = XXX_SETTING_DELEGATE.get();
    }
    return xxx;
}

setXXX(T newXXX) {
    xxx = newXXX;
    ... notifications ...
}

I don't think the derived/non-derived setting need affect the above.

---

Do you want to pick this up as an EMF SettingDelegate bug.

Otherwise at the OCL level, I have to workaround the missing cached value with an eAdapter.

Comment 3 Ed Merks 2013-10-09 01:52:32 EDT

So the more general problem is that there is no real support for stateful setting delegates because we call then like this

__ESETTING_DELEGATE.dynamicAbc(this, null, 0, ...) 

I.e., we never pass in a non-null DynamicValueHolder.

If something is to be changed in EMF, it should focus on such more general support, e.g., for the normal (non feature delegating) implementation pattern we could declare a field of type Object (it must support storing org.eclipse.emf.ecore.EStructuralFeature.Internal.DynamicValueHolder.NIL) and pass that along to all the setting delegate calls (or some other solution along those lines).

I don't see the need for special generation patterns like what you show in comment #2, because the setting delegate itself has support if "isSet" testing and the setting delegate should implement all necessary logic for determining what "get" returns.

Comment 4 Ed Willink 2013-10-09 12:57:03 EDT

Hi Ed

Yes general support for stateful setting delegates would significantly reduce the derived customization.

Comment 5 Ed Willink 2013-10-23 12:35:46 EDT

The underlying EMF evolution is likely to appear as part of Bug 405065.

Comment 6 Christian Damus 2014-01-14 10:34:54 EST

I have pushed a Gerrit review:

https://git.eclipse.org/r/20617

This implements, in the OCLSettingsDelegate, a cache of settings for initial-value features (being those that have computed initial values and are changeable).

This implementation makes the original reporter's test case work as expected.

However, this implementation so far only supports single-valued features.  Multi-valued features would need some elaboration:

  - the delegate's get() would have to return a mutable EList
  - this mutable EList would have to broadcast the appropriate notifications
    when it is changed
  - it would also have to put itself into the settings cache as an explicitly
    overridden value when it has been changed

But, it would be nice to evaluate this general approach before doing any such elaboration.

Comment 7 Ed Willink 2014-01-14 12:43:55 EST

(In reply to Christian W. Damus from comment #6)
> This implements, in the OCLSettingsDelegate, a cache of settings for
> initial-value features (being those that have computed initial values and
> are changeable).

I must be missing something.

The original problem was that Genmodel gave us an UnsupportedOperationException thrower. Once that's fixed, do we actually need to do anything? we could just recompute every time, but obviously a boolean flag (perhaps isInitialValue) can ensure that we do it on the first get() only.

Why do we need fancy caches when we inherit a nice changeable field? if volatile we recompute, if non-volatile we use the field as a cache.

Comment 8 Christian Damus 2014-01-14 12:57:26 EST

(In reply to Ed Willink from comment #7)
>
> I must be missing something.
> 
> The original problem was that Genmodel gave us an
> UnsupportedOperationException thrower. Once that's fixed, do we actually
> need to do anything? we could just recompute every time, but obviously a

If that's the problem we want to fix.  And who will fix it?  It seems to me that Ed M. doesn't like the field-backed-by-a-delegate pattern, so the solution would end up requiring codegen changes to invoke the delegates differently (with value-holders and real feature IDs) and changes in the delegates to implement statefulness.

This adapter approach could be simpler, though it is OCL-specific.  Perhaps it's useful just as a discussion point, but it could be a fall-back position in case other avenues don't pan out.  I'm not particularly attached to it. :-)

Comment 9 Ed Willink 2014-01-14 13:17:13 EST

(In reply to Christian W. Damus from comment #8)
> If that's the problem we want to fix.  And who will fix it?  It seems to me
> that Ed M. doesn't like the field-backed-by-a-delegate pattern, so the
> solution would end up requiring codegen changes to invoke the delegates
> differently (with value-holders and real feature IDs) and changes in the
> delegates to implement statefulness.

My recollection is that Ed M did not like adding support for the narrow OCL requirement in which the EMF-generated logic did the first time do-we-initialize decision, but was happy with a general solution in which the setting delegate got invoked with non-null holder rather than just throwing an UnsupportedOperation.

Whether the property field exists under normal volatile/non-volatile control is perhaps a grey area. It would seem to be simpler to have a normal rather than abnormal field so that we don't need to replicate the logic for numerous varieties of EMF List.

Comment 10 Ed Willink 2014-05-28 02:24:34 EDT

UML2 has made major progress in Property value support:  Bug 405065#c22

All OCL Property value bugs should be reviewed and hopefully closed/amalgamated as one.

Comment 11 Ed Willink 2015-05-23 17:13:07 EDT

(In reply to Ed Willink from comment #5)
> The underlying EMF evolution is likely to appear as part of Bug 405065.

Didn't happen.

(In reply to Ed Merks from comment #3)

> So the more general problem is that there is no real support for stateful
> setting delegates because we call then like this
> 
> __ESETTING_DELEGATE.dynamicAbc(this, null, 0, ...) 

although the null provokes the problem, the __ESETTING_DELEGATE was created by the OCLSettingDelegateFactory, so it is trivial to detect a non-volatile, changeable feature and create an OCLSettingDelegate.Changeable with an owner=>value map for the feature; much easier than an eAdapter.

The JUnit tests demontrate that the previous UnsupportedOperationException is now replaced by something useful.

Pushed to branch ewillink/418716.

Adolfo: please review.

Comment 12 Ed Willink 2015-05-25 16:26:40 EDT

Pushed to master for RC2.