Community
Participate
Working Groups
I don't think it is appropriate to throw an AssertionError in LazyLinkingResource#handleCyclicResolution(). The reason being that an Error would for instance cause the builder to abort ungracefully. There will for instance not be any error logged indicating which resource the error pertains to. I think it would be better to throw an IllegalStateException. Any other opinions on this?
EMF will catch those exceptions and simply return the unresolved proxy. We could try to make the assertion error more descriptive and log it properly in the builder context.
see org.eclipse.emf.ecore.util.EcoreUtil.resolve(EObject, ResourceSet) which is used by org.eclipse.emf.ecore.impl.BasicEObjectImpl.eResolveProxy(InternalEObject)
I can only see that EcoreUtil#resolve() handles RuntimeException, not Error. Further we have another point of entry: EcoreUtil2#resolveLazyCrossReferences() which doesn't handle any exceptions. But what is the reasoning for using an Error rather than a RuntimeException (like IllegalStateException) here?
(In reply to comment #3) > But what is the reasoning for using an Error rather than a RuntimeException > (like IllegalStateException) here? An unexpected cylcic resolution is _most_ likely a programming error thus we want to notify the developer about it. When writing tests you'll probably use a resource and navigate that by means of plain feature calls. These will resolve proxies (EcoreUtil#resolve) and if you run into cyclic resolution problems, the only way to report them is throwing something which is not a RuntimeException because those will be caught and you get an unresolved proxy and have no idea on what went wrong. Therefore we throw an error. Regarding EcoreUtil2#resolveLazyCrossReferences() I'd rather catch RuntimeException there in order to be consistent with EcoreUtil#resolve.
OK, now I get your point about EcoreUtil#resolve(). I suppose the alternative would be to log an error in LazyLinkingResource#handleCyclicResolution() and then throw an IllegalStateException. But I now agree with you that an AssertionError is quite appropriate here. In our scenario the language had just not been tested well enough yet. But I think it would be a good idea to include the resource's URI in the error message. That way it would at least get logged at some point with a reference to the resource that caused it.
We should definitely guard the builder such that it's not affected in this case. I think that's the main problem here.
I think also other logged error messages would benefit from including the resource URI. E.g. the two calls to Logger#error() in LazyLinkingResource#getEObject().
XbaseResource can also throw AssertionErrors. See bug 368263 for details. This will also lead to a canceled build and in the case of the mentioned bug the builder participant will thus also be skipped.
https://git.eclipse.org/r/30567
