Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: improve meta-metamodel classes about Spoon types and roles #1894

Merged
merged 3 commits into from
Mar 7, 2018

Conversation

pvojtechovsky
Copy link
Collaborator

This PR

@pvojtechovsky
Copy link
Collaborator Author

@monperrus I guess it is not correct that CI requires javadoc on methods of PRIVATE interface. See 6c251e4

@pvojtechovsky
Copy link
Collaborator Author

Implemenetation of this PR based on code generated by test meta model (SpoonMetaModel) has some problems:

  • it needs more maintenance - the change in model has to be adapted manually in Metamodel#initTypes
  • it is another source of metamodel information, so it might be confusing what is primary source of Spoon meta model information (actually they are: annotations on ifaces and implementations and CtScanner)

So the solution, which I would probably like the most would be to move SpoonMetaModel from the test to main (runtime) + adapt that API so it is more nice ...

WDYT?

By the way, do you agree that having a well understandable, fast and precise facade around spoon metamodel (something like SpoonMetaModel) would be helpful for clients and for us, because it allows to implement great new Spoon features?

@monperrus
Copy link
Collaborator

I agree that having access to the metamodel at runtime is useful.
There are two options for doing this.

  1. Option 1: Introducing specific classes for this, such as MMType, etc
  2. Option 2: Reusing Spoon's classes: a metamodel class is represented as a CtType directly.

I prefer option 2 because:

  • it is conceptually elegant, because it's meta-circular
  • we eat our own doog food with respect to shadow classes
  • we have access to all annotations because they have runtime retention.
  • we already have a first version of it with Metamodel#getAllMetamodelInterfaces

What would you like to do that is not possible with the shadow CtType of getAllMetamodelInterfaces?

@pvojtechovsky
Copy link
Collaborator Author

I agree that having access to the metamodel at runtime is useful.

Super so we are on the same side ;-)
And thanks for discussing that, because I have no clear vision how to do it well.

I prefer option 2 because: ...

I agree with all these reasons, but there is one problem: Spoon's CtType is very hard to use for that purpose.

For example, the primitive feature, which I need to finish #1686, Metamodel#isDerivedRoleInClass(CtRole, Class) needs about 30 lines of code + good testing. It is really not that simple like, just take a getter declaration and check the @DerivedProperty annotation.... See MMField#isDerived for details. May be I am wrong and it can be done in a simpler way, but then I really need a help + running test, which shows that simple code returns same results like MMField#isDerived.
... and if I need a help, how other new Spoon contributors might write that code / or understand that code?

So I suggest

  • to introduce specific (design pattern Facade) classes for that (like MMType), which SHOULD internally use CtType - to keep single primary source of these metadata => annotations on spoon classes.
  • to test them well, so we can trust them and use them later to generate spoon code itself
  • to cache the computed metadata - so it is fast enough when e.g. matching/generating spoon code will ask isDerived for each to be checked property of each matched combination .... many many times ...
  • to have this code available in runtime (not only for tests, where MMType already works well)
  • to share as much code with MMType, so we do not duplicate algorithms/maintenance

Do you understand problem? Do you agree to move code from SpoonMetaModel (test only) to Metamodel (runtime)?

@monperrus
Copy link
Collaborator

monperrus commented Mar 4, 2018

I agree to have one single version of the metamodel both from runtime and test purposes. But it's probably not the current SpoonMetaModel, because its naming, its design and its public methods have to be improved first (it was not designed and reviewed for being part of the public API).

to test them well, so we can trust them and use them later to generate spoon code itself

Yes, of course :-)

to cache the computed metadata - so it is fast enough when e.g. matching/generating spoon code will ask isDerived for each to be checked property of each matched combination .... many many times ...

Yes, this can come later in a subsequent API, once we get the basic API right

to introduce specific (design pattern Facade) classes for that (like MMType), which SHOULD internally use CtType - to keep single primary source of these metadata => annotations on spoon classes.

Here, instead of delegating, I would propose to extend CtType (and the other appropriate classes), so that it is still metacircular: a metamodel type is a CtType plus some additional metamodel-specific methods such as isDerivedRoleInClass

@pvojtechovsky
Copy link
Collaborator Author

SpoonMetaModel, because its naming, its design and its public methods have to be improved first

yes, I am aware of that. I agree to refactor it as needed. I also understand current SpoonMetaModel more like a prototype/experimental.... but already quite useful ;-)

instead of delegating, I would propose to extend CtType

I do not understand why. We probably speak about different metamodels. My mental image of Spoon metamodel consists of entities like

  • MMType where 1 instance of MMType is mapped for example to 1 java interface CtVariable and 1 java class CtVariableImpl
  • MMField where 1 instance of MMField is mapped to 1 CtRole and to many methods, which provides access to value of that role.

Such high level meta model doesn't extend java meta model, because it is language independent. So it makes no sense to MMType extends CtType in this case. Nearly no feature of CtType makes sense for MMType, so why to extend?

... and on all the places where SpoonMetaModel is already used in testing and generation, there was needed such high level metamodel.

So may be you think about something else? What would be

And I wonder about one think. How that meta model type which extends CtTypeImpl/CtClassImpl/CtInterfaceImpl would be created?
A) compiler would detect that it is meta model and it would directly create it
B) we would create it manually by a special kind of clone of normal CtTypeImpl?
C) ?

@monperrus
Copy link
Collaborator

MMType where 1 instance of MMType is mapped for example to 1 java interface CtVariable and 1 java class CtVariableImpl
MMField where 1 instance of MMField is mapped to 1 CtRole and to many methods, which provides access to value of that role.

Now I see what you mean. Those two concepts/definitions make sense. Maybe we could first agree on names for those concepts, and refactor them in src/test to start with.
AFAIU your idea I would propose:

  • MMType -> MetamodelConcept (no confusion with Java types or Spoon CtType)
  • MMField -> MetamodelProperty (no confusion with Java fields)

@pvojtechovsky
Copy link
Collaborator Author

Super! So

  • I rolled back the changes concerning runtime metamodel (hoping that test metamodel will evolve to be moved to runtime)
  • I renamed as you suggest. Including some fields and comments.

What next? May be some parts of test model are experimental ... but these are needed:

  • MMTypeKind. What name do you suggest?
  • MMContainerType can be replaced by existing spoon.reflect.meta.ContainerKind ... I will do it soon

Concerning MMMethod and MMMethodKind, they are actually used

  • internally in metamodel to compute some necessary data
  • for generating of RoleHandler
  • for testing of CtScanner
    So it would be OK for me to hide them somehow ... but how?

@monperrus
Copy link
Collaborator

monperrus commented Mar 7, 2018 via email

@pvojtechovsky
Copy link
Collaborator Author

I agree, with merge of this one.

To go over the public methods and see whether we are OK with the design

I guess it is task for you. I can refactor it then when you found something.

@monperrus monperrus merged commit b957c63 into INRIA:master Mar 7, 2018
@monperrus monperrus changed the title feat: Metamodel provides metadata about Spoon types and fields refactor: improve meta-metamodel classes about Spoon types and fields Mar 7, 2018
@monperrus monperrus changed the title refactor: improve meta-metamodel classes about Spoon types and fields refactor: improve meta-metamodel classes about Spoon types and roles Mar 7, 2018
@pvojtechovsky pvojtechovsky deleted the feaMetaInfo branch March 8, 2018 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants