review: feature: add support for implicit / explicit modifiers (CtExtendedModifier)

[surli]

No description provided.

[surli]

I have a semantic problem here: technically a field declared in an interface like I did:

int MY_CONSTANT = 42;

does not have an explicit modifier.
However, as it's in an interface, it's a static final public field.
For the visibility I may be able to use getVisibility but, I don't think I have an easy way to get the other information: final and static.

So should we put the information in modifier, and then we should be able to use hasModifier as for any other field, or should we treat these fields differently?

WDYT @monperrus @pvojtechovsky ?

[pvojtechovsky]

What about this:

• keep behavior of modifiers unchanged - reflecting the existence of real keyword in source code.
• Add new methods: isPublic, isFinal whose result would be computed depending on container type.

WDYT?

[surli]

I agree on the first one:

keep behavior of modifiers unchanged - reflecting the existence of real keyword in source code.

For the second one, it means to add isPublic, isStatic, isFinal, at least.
Another solution could be to differenciate sourceModifiers from effectiveModifiers?...

[pvojtechovsky]

to differenciate sourceModifiers from effectiveModifiers

it is acceptable for me too.

Actually I have feeling like isPublic, isFinal, ... methods would be easier to implement and easier to user for clients too. Anyway both solutions are OK for me.

WDYT?

[surli]

Actually I have feeling like isPublic, isFinal, ... methods would be easier to implement and easier to user for clients too.

Easier to implement, no because they're already implemented in fact: the changes I made here should be applied to effective modifiers, and I should use the old behaviour to source modifiers.

And for client, I'm mixed: it's easier to find a method called isPublic than hasModifier, but when you know both it requires to look into it to understand why they do not necessarly return the same result. Using two explicit hasModifier would avoid this misunderstanding.
We could also benefit from both solution: creating hasSourceModifier, hasEffectiveModifier, and isPublic, isFinal proxying to hasEffectiveModifier. Then we would be explicit and easy to use.

[monperrus]

keep behavior of modifiers unchanged - reflecting the existence of real keyword in source code.

I would disagree. This would be against one of the core design principle of Spoon: it provides an abstract AST close to the semantic of the program, it does not simply / only reflect the source code.

[pvojtechovsky]

Spoon: it provides an abstract AST close to the semantic of the program

therefore we suggest to create new isPublic, isFinal, ..., (or getEffectiveModifiers) whose results would be always computed following semantic of the program.

But please keep origin methods unchanged

• to be backward compatible
• to be able to pretty print model back to sources as much similar as origin sources
• to give client chance to influence what is pretty printed - they (including me) are asking for that ...

[monperrus]

we suggest to create new isPublic, isFinal, ..., whose results would be always computed following semantic of the program

that's indeed a good idea, created #1657

But please keep original behavior unchanged

I'm not sure about this, There is a bug in the model itself for modifiers of interface members. Could you elaborate on how fixing the bug would break your code?

[pvojtechovsky]

What bug? Do you mean that getModifiers() does not return PUBLIC on the field of interface?
OK, I understand. The clients and even Spoon code itself already expects that getModifiers() returns effective modifiers (computed from scope).

So then I like solution with getModifiers() and getSourceModifiers(), where new getSourceModifiers() would return modifiers which were really used in source code.

WDYT?

[monperrus]

OK, I understand. The clients and even Spoon code itself already expects that getModifiers() returns effective modifiers (computed from scope).

Excellent, we're on the same wavelength.

So then I like solution with getModifiers() and getSourceModifiers(), where new getSourceModifiers() would return modifiers which were really used in source code.

That's a good idea, added to #1657

[surli]

OK, I understand. The clients and even Spoon code itself already expects that getModifiers() returns effective modifiers (computed from scope).

Indeed. The javadoc of CtModifiable#getModifiers confirms that:

• Returns the modifiers of this element, excluding annotations. Implicit
  * modifiers, such as the {@code public} and {@code static} modifiers of
  * interface members, are included.

[surli]

In order to avoid breaking lot of tests, I decided to directly put getSourceModifiers in CtModifiable (else we break everything is based on pretty printer...).

I still had to make lot of changes:

1. addModifiers and removeModifiers both acts on modifiers and sourceModifiers
2. in ElementPrinterHelper#writeModifiers it was implicitely considered that getModifiers return a sorted list of modifiers: apparently it's not the case anymore so I had to "sort" modifiers there before printing
3. when calling setModifiers it does not call to addModifier anymore (because it put thing also in sourceModifiers), then I change the listener to use updateObject
4. I also call updateObject in setSourceModifiers

Tell me if those design choices are OK. If so I continue by checking in the different add/setModifiers what should be checked or not and maybe by adding more tests.

[monperrus]

I would say that we should not have setSourceModifiers in the interface. This notion of "source" make only sense fo JDTTreeBuilder and is meaningless to be set for clients.

Actually, I really see here a risk of maintaining some state twice, which is really hard. Having the notion of implicitness on top of modifier kinds would be better and avoid to maintain state and consistency.

[pvojtechovsky]

This notion of "source" make only sense fo JDTTreeBuilder

no. It makes sense for pretty printing too. I think that clients want to have control on whether the "public" keyword is printed for interface field or not. I am one of these clients too. Same like they want to have control on extra new lines between statements.

It is not maintaining of double state. We will keep one state - source modifiers - and compute the effective state from the context.

[monperrus]

We agree that we don't want to maintain twice the same state. What I don't like with "source" is that it is ambiguous: does it refer to input source or output source. I'd propose to call that "ExtendedModifier"

What about this:

• we add CtExtendedModifier

class CtExtendedModifier {
  boolean isImplicit
  ModifierKind kind
}

• we replace List<ModifierKind> modifiers CtList<CtExtendedModifier> modifiers
• we add set/getExtendedModifiers in CtModifiable
• add/set/getModifiers would simply box/unbox a ModifierKind in a CtExtendedModifier

WDYT?

[surli]

I'm ok with that changes. Ok too @pvojtechovsky ?

[pvojtechovsky]

I like this solution too. But please do not implement it before we merge all the CtRole and CtScanner ... related PRs, because it would break them - heavy merging would be needed.

[monperrus]

Thanks a lot @surli, we have a good solution here.

For the code review, it's hard to see the new tests and the modified assertions, because the tests changed too much. Did you add the tests related to the bug and the JUnit regression?

My only concern is about the new factory method createCatchVariable(extendedModifiers) which seems very specific and hardly meant to be used by clients. What about dropping the new method and simply calling createCatchVariable().setExtendedModifiers?

[surli]

For the code review, it's hard to see the new tests and the modified assertions, because the tests changed too much.

That's because I renamed the old test file and reuse the old name for a new test case: the diff is badly displayed. I will put back the old name for the proper test file to have a better diff.

Did you add the tests related to the bug and the JUnit regression?

I created #1690 for the problem related to JUnit, as I wasn't completely sure it was related to this one. Once this one is merged, I will rebase #1690 to check if the test is passing or if it's another bug.

What about dropping the new method and simply calling createCatchVariable().setExtendedModifiers?

ok with that

[monperrus]

I will put back the old name for the proper test file to have a better diff.

super, thanks.

[monperrus]

add assertions in isImplicit?

[monperrus]

add assertions on pretty-printing implicit modifiers?

[surli]

@monperrus just fix the test and code. For me it's ready.

[monperrus]

Thanks.

Could you:

• add assertions of method isImplicit?
• add assertions on pretty-printing of implicit modifiers? (should not be pretty-printed)

[surli]

Could you:

add assertions of method isImplicit?
add assertions on pretty-printing of implicit modifiers? (should not be pretty-printed)

Done.

[spoon-bot]

Detected changes by Revapi: 3.

Old API: fr.inria.gforge.spoon:spoon-core:jar:6.0.0-20171107.084259-149

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

Name	Change 1
Old	none
New	method spoon.reflect.declaration.CtElement spoon.reflect.factory.Factory::createElement(java.lang.Class<? extends spoon.reflect.declaration.CtElement>)
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking,

Name	Change 2
Old	none
New	method java.util.Set<spoon.support.reflect.CtExtendedModifier> spoon.reflect.declaration.CtModifiable::getExtendedModifiers()
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking,

Name	Change 3
Old	none
New	method T spoon.reflect.declaration.CtModifiable::setExtendedModifiers(java.util.Set<spoon.support.reflect.CtExtendedModifier>)
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking,

[monperrus]

thanks