Add adapters to JvmTypeReference to express the expected semantics of supertypes #2924
Conversation
| @@ -391,10 +392,14 @@ protected void initialize(XtendClass source, JvmGenericType inferredJvmType) { | |||
| if (typeRefToObject != null) | |||
| inferredJvmType.getSuperTypes().add(typeRefToObject); | |||
| } else { | |||
| inferredJvmType.getSuperTypes().add(jvmTypesBuilder.cloneWithProxies(extendsClause)); | |||
| JvmTypeReference superClass = jvmTypesBuilder.cloneWithProxies(extendsClause); | |||
There was a problem hiding this comment.
API wise:
What do you think about
jvmTypesBuilder.addImplementedInterface(inferredJvmType, superInterface);
jvmTypesBuilder.setSuperClass(inferredJvmType, superClass);
Internally that would do cloneWithProxies and add the adapter.
There was a problem hiding this comment.
I like it! :)
I would call the first one addSuperInterface though since for an interface implemented is misleading: in that case it should be extended. With SuperIterface we could cover both cases, right?
| } | ||
|
|
||
| public static boolean isExpectedAsClass(JvmTypeReference typeReference) { | ||
| return typeReference.eAdapters().stream().anyMatch(ExpectedClassMarker.class::isInstance); |
There was a problem hiding this comment.
Preferably let the adapter implement isAdapterForType and use EcoreUtils.getAdapter(..) It avoids some allocations and is more in line with the "usual" EMF way of doing this.
There was a problem hiding this comment.
Right! I took inspiration from DisplayHelper, maybe that should be changed as well.
I think we could also use singletons for the two adapters.
| } | ||
| } | ||
|
|
||
| private static class ExpectedInterfaceMarker extends AdapterImpl { |
There was a problem hiding this comment.
AdapterImpl has a field Notifier target that is maintained. I don't think we need that and it should be fine use an implementation similar to org.eclipse.xtext.xbase.validation.ReadAndWriteTracking.READMARKER
There was a problem hiding this comment.
OK, so simply check the singleton is there.
I'll do that
There was a problem hiding this comment.
I now use the same strategy of org.eclipse.xtext.xbase.validation.ReadAndWriteTracking.READMARKER in 4b407b4
| /** | ||
| * @author Lorenzo Bettini - Initial contribution and API | ||
| */ | ||
| public class JvmTypeReferenceUtil { |
There was a problem hiding this comment.
I don't really like JvmTypeReferenceUtil next to TypeReferences. Would you think these two classes could be merged (only keeping TypeReferences)?
There was a problem hiding this comment.
I can put the methods in TypeReferences, as instance methods in that case.
I haven't found a TypeReferencesTest so I'll create that and move also the tests of JvmTypeReferenceUtilTest there
There was a problem hiding this comment.
I don't really like
JvmTypeReferenceUtilnext toTypeReferences. Would you think these two classes could be merged (only keeping TypeReferences)?
@szarnekow I checked TypeReferences, which is based on injection. The new util class doesn't need injection, that's why I created static methods. Does adding the static methods as instance methods in TypeReferences still make sense? Or as instance methods? For testing, I seem to understand the common.types.tests don't use injection. I could still create an instance of TypeReferences with new to test the new methods since I wouldn't use other methods from that class. Just to know whether it still looks fine.
There was a problem hiding this comment.
You're right. I missed that TypeReferences has quite a few dependencies. The approach based on its own utility class JvmTypeReferenceUtil might be the better option indeed.
| @@ -32,8 +32,7 @@ class DomainmodelJvmModelInferrer extends AbstractModelInferrer { | |||
| def dispatch infer(Entity entity, extension IJvmDeclaredTypeAcceptor acceptor, boolean prelinkingPhase) { | |||
| accept(entity.toClass( entity.fullyQualifiedName )) [ | |||
| documentation = entity.documentation | |||
| if (entity.superType !== null) | |||
| superTypes += entity.superType.cloneWithProxies | |||
| superClass = entity.superType | |||
There was a problem hiding this comment.
@szarnekow just a small note: by using Xbase syntactic sugar, the use of the new API looks like above.
Is it better to be more explicit in this example and call setSuperClass(entity.superType)? We'd still use the implicit it anyway.
There was a problem hiding this comment.
I like it with the regular assignment syntax. You're not a fan?
There was a problem hiding this comment.
I'm a fan as well, I just wanted to make sure :)
OK, so when the build is green I'll merge
to set a JvmTypeRefefences as an expected class/interface
This will make it easier to be used, without checking for null in advance
also added a compiler test for superclass presence, which was never tested before
LorenzoBettini
force-pushed
the
lb_2880-3
branch
from
4b407b4
to
90c17f2
Compare
Closes #2880
This is an alternative to #2899 that doesn't touch the Jvm types model: it introduces a new utility class to mark a JvmTypeReference as an expected class or interface, using adapters.
Currently, this information is never used, but it will be useful for #2349
I also applied that to XtendJvmModelInferrer. I'm not too happy with the introduced API, which requires saving the cloned type reference in a local variable, use the new utility class, and then use the type reference.
To make the API better I can think of two possible solutions:
JvmTypeReferenceUtil"fluent" to return the referencecloneWithProxiesExpectedAsClass,cloneWithProxiesExpectedAsInterface