refactor(role): add and refactor CtRole annotations of model #1606

pvojtechovsky · 2017-10-15T19:42:50Z

See individual commits of this PR. Each of them might be individual PR, but they often depends on each other, so their order is relevant.

I suggest:

please check each commit individually
let me know which changes are merge-able and I will make another PR for these commits only.
remaining changes (if any) can be then discussed individually in separate PRs.

This PR contains commits of #1600 and #1605, because other commits depends on them.

pvojtechovsky · 2017-10-15T20:15:59Z

Some notest about changes:

fix(role): CtPackage.types is CtRole.CONTAINED_TYPES
replace forgotten CtRole.TYPE in CtPackageImpl by CONTAINED_TYPES

See #1600

fix(role): replace CtJavaDocTag CtRole.TYPE by DOCUMENTATION_TYPE

Suggested by Martin in #1582

fix(role): replace CtJavaDocTag CtRole.PARAMETER by JAVADOC_TAG_VALUE

Suggested by Martin in #1582

fix(role): replace CtExecutable CtRole.THROWN by THROWN_TYPE

Suggested by Martin in #1582

fix(role): replace CtRole.EXECUTABLE by METHOD and ANNONYMOUS_EXECUTABLE

Methods: CtClass#getAnonymousExecutables(), etc. were not marked with any role, because the only matching role was EXECUTABLE, which was already used for CtType@getMethods(), etc.
So I have replaced CtRole.EXECUTABLE (which already provided access to CtType.getMethods() only) by new role METHOD.
I have added new role ANNONYMOUS_EXECUTABLE and used it on places where it was missing or where there was used wrong EXECUTABLE role

fix: wrong CtRole.EXECUTABLE in CtNewClassImpl#setAnonymousClass

See #1605 - it is just small fix of a wrong use of role

fix(role): rename CtRole.EXECUTABLE to CtRole.EXECUTABLE_REF

Suggested by Martin in #1582

tdurieux · 2017-10-15T20:53:00Z

src/main/java/spoon/reflect/declaration/CtClass.java

@@ -44,6 +45,7 @@
 	 * Derived from {@link #getTypeMembers()}
 	 */
 	@DerivedProperty


remove DerivedProperty?

CtClass#getConstructor has already derived property too. So I kept it. But I do not know whether it belongs there or not. I will remove it if you are sure that it has to be removed - no problem.
WDYT @surli @tdurieux

getAnonymousExecutables is not a Derived Property. We should remove it.

And what about CtClass#getConstructor ?

getConstructor(CtTypeReference<?>... parameterTypes) is not a real getter for me.
It is more a "finder".

According to the specification @interface DerivedProperty, getConstructors() is a derived property because the content is already scanned by getTypeMembers()

For me in this case, getTypeMembers() is DerivedProperty and getConstructors, getExecutables, ... are not.
it is just an implementation decision to use getTypeMembers instead of the other methods.
Constructors, methods, ... are the real properties of the model

Constructors, methods, ... are the real properties of the model

I agree, but the order is defined on the level of typeMember ... so typeMember is real property too.

We need something in between ...

We should first try to make a conclusion of the purpose of the CtRole and definition of contract behind the CtRole. I guess each of us understands it different ;-p

So after discussion below it is probably clear that @DerivedProperty should stay there. So this PR is correct at this place.

monperrus · 2017-10-16T13:17:58Z

We should add a test to check that elements are never scanned twice by two different getters (whatever they are). And then definition of a CoreProperty is that it is called directly by CtScanner (as opposed to a derived property)

monperrus · 2017-10-16T14:01:27Z

We should first try to make a conclusion of the purpose of the CtRole and definition of contract behind the CtRole.

At least three usages (which make roles so interesting): the model intercession listener (by @tdurieux), the path queries, the get/setValueByRole (your current work)

pvojtechovsky · 2017-10-16T14:09:09Z

I think the main conflict is here
Type has typeMembers.

typeMembers
- field
- nested type
- executable
  - constructor
  - method
  - annonymousExectuable

How to map roles on that?
A) to have role for each bullet
B) to have role for top typeMember only
C) to have role for leafs only
D) ?

monperrus · 2017-10-16T14:15:17Z

I would say on the leafs only. But roles and DerivedProperty are orthogonal, right?

…

On 10/16/17 4:09 PM, Pavel Vojtechovsky wrote: I think the main conflict is here Type has typeMembers. * typeMembers o field o nested type o executable + constructor + method + annonymousExectuable How to map roles on that? A) to role for each bullet B) to have role for top |typeMember| only C) to have role for leafs only D) ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1606 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAxDUtRlD6NI2m3DJ_Yy5VYu1ZfqFyq9ks5ss2OGgaJpZM4P53b->.

pvojtechovsky · 2017-10-16T14:28:55Z

orthogonal

I am sorry, but I am not well trained in math/graph terms - and understanding wiki explanation of this term would need hours for me.

I guess we do not have to make any relation between roles and DerivedProperty. DerivedProperty is well defined and is used for checking of scanner. Let's keep it as it is.

And for roles we can do it as we want. For me it is no problem if roles will overlap, so I can live with all these roles: TYPE_MEMBER, EXECUTABLE, FIELD, METHOD, ... depending on role it will get/set appropritate values - no problem that it overlaps.
Just the model intercession listener has to use it consistently
A) on root only - TYPE_MEMBER
B) on leaves only - as it is now after my latest PR
the other roles will be never used by model intercession listener - no problem, these roles are for other purposes

pvojtechovsky · 2017-10-16T16:54:49Z

We should add a test to check that elements are never scanned twice by two different getters
(whatever they are).

This test already exist. See MainTest#checkModelIsTree. If any element would be visited twice then this test would fail.

pvojtechovsky · 2017-10-16T18:30:26Z

Here is current state of Type - CtRole assignments
concept.txt

I suggest to create new role for these:

CtCodeSnippet CtRole.EXPRESSION
	ItemType: java.lang.String
	get: getValue() : java.lang.String
	set: setValue(java.lang.String)
----------------------------------------------------------
CtForEach CtRole.VARIABLE
	ItemType: spoon.reflect.code.CtLocalVariable<?>
	get: getVariable() : spoon.reflect.code.CtLocalVariable<?>
	set: setVariable(spoon.reflect.code.CtLocalVariable)

because these roles are already used with different meaning.

I suggest to replace CtRole.THROWN by CtRole.EXPRESSION

CtThrow CtRole.THROWN
	ItemType: spoon.reflect.code.CtExpression<? extends java.lang.Throwable>
	get: getThrownExpression() : spoon.reflect.code.CtExpression<? extends java.lang.Throwable>
	set: setThrownExpression(spoon.reflect.code.CtExpression)

because we do not need special role for that. It will be then compatible with many other commands like:

CtReturn CtRole.EXPRESSION
	ItemType: spoon.reflect.code.CtExpression<R>
	get: getReturnedExpression() : spoon.reflect.code.CtExpression<R>
	set: setReturnedExpression(spoon.reflect.code.CtExpression)

@monperrus suggested to change CtExecutableReference CtRole.PARAMETER to CtExecutableReference CtRole.PARAMETER_REF (or .PARAMETER_TYPE)

CtExecutable CtRole.PARAMETER
	ItemType: java.util.List<spoon.reflect.declaration.CtParameter<?>>
	get: getParameters() : java.util.List<spoon.reflect.declaration.CtParameter<?>>
	set: setParameters(java.util.List)
	add: addParameter(spoon.reflect.declaration.CtParameter)
----------------------------------------------------------
CtExecutableReference CtRole.PARAMETER
	ItemType: java.util.List<spoon.reflect.reference.CtTypeReference<?>>
	get: getParameters() : java.util.List<spoon.reflect.reference.CtTypeReference<?>>
	set: setParameters(java.util.List)

note that CtRole.TYPE_PARAMETER has similar contract

CtActualTypeContainer CtRole.TYPE_PARAMETER
	ItemType: java.util.List<spoon.reflect.reference.CtTypeReference<?>>
	get: getActualTypeArguments() : java.util.List<spoon.reflect.reference.CtTypeReference<?>>
	set: setActualTypeArguments(java.util.List)
----------------------------------------------------------
CtFormalTypeDeclarer CtRole.TYPE_PARAMETER
	ItemType: java.util.List<spoon.reflect.declaration.CtTypeParameter>
	get: getFormalCtTypeParameters() : java.util.List<spoon.reflect.declaration.CtTypeParameter>
	set: setFormalCtTypeParameters(java.util.List)
	add: addFormalCtTypeParameter(spoon.reflect.declaration.CtTypeParameter)
	remove: removeFormalCtTypeParameter(spoon.reflect.declaration.CtTypeParameter)

should CtActualTypeContainer CtRole.TYPE_PARAMETER be replaced too by some new role?

monperrus · 2017-10-17T16:19:51Z

I suggest to create new role for these: CtCodeSnippet CtRole.EXPRESSION, CtForEach CtRole.VARIABLE

I agree, I would remove the PropertyGetter on the first one (code snippets are more clever hacks than AST elements). For the second one, what role do you suggest?

pvojtechovsky · 2017-10-17T17:49:35Z

I would remove the PropertyGetter CtCodeSnippet CtRole.EXPRESSION

~~ok, I will do so~~

For the second one, what role do you suggest?

CtForEach CtRole.VARIABLE might be replaced to

CtRole.FOR_EACH_VARIABLE
CtRole.FOREACH_VARIABLE
CtRole.FOR_VARIABLE, //but that might be confusing with CtFor CtRole.FOR_INIT and CtRole.FOR_UPDATE

pvojtechovsky · 2017-10-17T17:57:47Z

Martin wrote
I would remove the PropertyGetter CtCodeSnippet CtRole.EXPRESSION

Do you mean only getter or setter too? I guess setter too. Then there is a problem ... this will influence setter implementation on

class CtCodeSnippetExpressionImpl/CtCodeSnippetStatementImpl {
...
	public <C extends CtCodeSnippet> C setValue(String value) {
		getFactory().getEnvironment().getModelChangeListener().onObjectUpdate(this, EXPRESSION, value, this.value);
		this.value = value;
		return (C) this;
	}

@tdurieux is it correct that model listeners will be NOT notified about changes on code snippets?

May be we should keep have some new role name (instead of removing getter/setter)

monperrus · 2017-10-17T19:15:44Z

I like CtRole.FOREACH_VARIABLE

spoon-bot · 2017-10-17T19:33:29Z

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:6.0.0-20171017.190052-88

New API: fr.inria.gforge.spoon:spoon-core:jar:6.0.0-SNAPSHOT

Detected changes: 1.

Change 1

Name	Element
Old	field spoon.reflect.path.CtRole.EXECUTABLE
New	none
Code	java.field.removed
Description	Field removed from class.
Breaking	binary: breaking,

pvojtechovsky · 2017-10-17T19:37:32Z

Here is current report of type to role mappings - concept.txt

pvojtechovsky · 2017-10-18T08:41:28Z

@monperrus what are next steps here? Will you merge it as it is and will we add other fixes in next PR? What about other suggestions here? Ignore them for now?

monperrus · 2017-10-18T09:11:49Z

It's indeed ready to be merged. I will merge it later today or tomorrow.

I believe that roles will be a super useful new feature, and this ongoing hot work is really good, thank you!

pvojtechovsky force-pushed the fixRoles branch from 51829f0 to 366ac74 Compare October 15, 2017 19:57

INRIA deleted a comment from spoon-bot Oct 15, 2017

tdurieux reviewed Oct 15, 2017

View reviewed changes

pvojtechovsky force-pushed the fixRoles branch from 366ac74 to a98f199 Compare October 16, 2017 05:09

INRIA deleted a comment from spoon-bot Oct 16, 2017

pvojtechovsky mentioned this pull request Oct 16, 2017

review: feat: Spoon CtRole based attributes access #1582

Merged

pvojtechovsky added 5 commits October 17, 2017 21:02

fix(role): replace CtJavaDocTag CtRole.TYPE by DOCUMENTATION_TYPE

b45a311

fix(role): replace CtJavaDocTag CtRole.PARAMETER by JAVADOC_TAG_VALUE

e37a79c

fix(role): replace CtExecutable CtRole.THROWN by THROWN_TYPE

51bde78

fix(role): replace CtRole.EXECUTABLE by METHOD and ANNONYMOUS_EXECUTABLE

d664169

fix(role): rename CtRole.EXECUTABLE to CtRole.EXECUTABLE_REF

46b1283

pvojtechovsky force-pushed the fixRoles branch from a98f199 to 46b1283 Compare October 17, 2017 19:03

replace CtForEach CtRole.VARIABLE by FOREACH_VARIABLE

3d5bc3b

INRIA deleted a comment from spoon-bot Oct 17, 2017

monperrus merged commit 40e15b2 into INRIA:master Oct 19, 2017

pvojtechovsky deleted the fixRoles branch October 28, 2017 19:50

surli mentioned this pull request Nov 14, 2017

[RELEASE] Major Spoon Release 6.0.0: done. #1699

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(role): add and refactor CtRole annotations of model #1606

refactor(role): add and refactor CtRole annotations of model #1606

pvojtechovsky commented Oct 15, 2017

pvojtechovsky commented Oct 15, 2017

tdurieux Oct 15, 2017

pvojtechovsky Oct 16, 2017

tdurieux Oct 16, 2017

pvojtechovsky Oct 16, 2017

tdurieux Oct 16, 2017

monperrus Oct 16, 2017

tdurieux Oct 16, 2017 •

edited

Loading

pvojtechovsky Oct 16, 2017

pvojtechovsky Oct 16, 2017

pvojtechovsky Oct 17, 2017

monperrus commented Oct 16, 2017 via email

monperrus commented Oct 16, 2017

pvojtechovsky commented Oct 16, 2017 •

edited

Loading

monperrus commented Oct 16, 2017 via email

pvojtechovsky commented Oct 16, 2017

pvojtechovsky commented Oct 16, 2017

pvojtechovsky commented Oct 16, 2017

monperrus commented Oct 17, 2017

pvojtechovsky commented Oct 17, 2017 •

edited

Loading

pvojtechovsky commented Oct 17, 2017

monperrus commented Oct 17, 2017 via email

spoon-bot commented Oct 17, 2017

pvojtechovsky commented Oct 17, 2017

pvojtechovsky commented Oct 18, 2017

monperrus commented Oct 18, 2017

refactor(role): add and refactor CtRole annotations of model #1606

refactor(role): add and refactor CtRole annotations of model #1606

Conversation

pvojtechovsky commented Oct 15, 2017

pvojtechovsky commented Oct 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdurieux Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monperrus commented Oct 16, 2017 via email

monperrus commented Oct 16, 2017

pvojtechovsky commented Oct 16, 2017 • edited Loading

monperrus commented Oct 16, 2017 via email

pvojtechovsky commented Oct 16, 2017

pvojtechovsky commented Oct 16, 2017

pvojtechovsky commented Oct 16, 2017

monperrus commented Oct 17, 2017

pvojtechovsky commented Oct 17, 2017 • edited Loading

pvojtechovsky commented Oct 17, 2017

monperrus commented Oct 17, 2017 via email

spoon-bot commented Oct 17, 2017

Revapi Analysis results

Change 1

pvojtechovsky commented Oct 17, 2017

pvojtechovsky commented Oct 18, 2017

monperrus commented Oct 18, 2017

tdurieux Oct 16, 2017 •

edited

Loading

pvojtechovsky commented Oct 16, 2017 •

edited

Loading

pvojtechovsky commented Oct 17, 2017 •

edited

Loading