review refactor: all non-leaf interfaces of the metamodel should be visited by CtInheritanceScanner #1703

surli · 2017-11-09T13:08:36Z

…should be visited in CtScanner

surli · 2017-11-09T13:29:17Z

every abstract non-leaf CtElement

but how do you define a leaf CtElement? If I'm comparing against the package location, there is some interfaces in spoon.reflect.code which are non leaf. It's not perfectly clear for me which interface should be scannable and which one should not be. I currently get the following list of missing method in CtInheritanceScanner:

scanCtAnnotationFieldAccess scanCtArrayRead scanCtArrayWrite scanCtAssert scanCtAssignment scanCtBinaryOperator scanCtBlock scanCtBreak scanCtCase scanCtCatch scanCtCatchVariable scanCtCodeSnippetExpression scanCtCodeSnippetStatement scanCtComment scanCtConditional scanCtConstructorCall scanCtContinue scanCtDo scanCtExecutableReferenceExpression scanCtFieldRead scanCtFieldWrite scanCtFor scanCtForEach scanCtIf scanCtInvocation scanCtJavaDoc scanCtLambda scanCtLiteral scanCtLocalVariable scanCtNewArray scanCtNewClass scanCtOperatorAssignment scanCtReturn scanCtStatementList scanCtSuperAccess scanCtSwitch scanCtSynchronized scanCtThisAccess scanCtThrow scanCtTry scanCtTryWithResource scanCtTypeAccess scanCtUnaryOperator scanCtVariableRead scanCtVariableWrite scanCtWhile scanCtAnnotation scanCtAnnotationMethod scanCtAnnotationType scanCtAnonymousExecutable scanCtClass scanCtConstructor scanCtEnum scanCtEnumValue scanCtField scanCtInterface scanCtMethod scanCtPackage scanCtParameter scanCtTypeParameter scanCtArrayTypeReference scanCtCatchVariableReference scanCtExecutableReference scanCtFieldReference scanCtIntersectionTypeReference scanCtLocalVariableReference scanCtPackageReference scanCtParameterReference scanCtTypeParameterReference scanCtTypeReference scanCtUnboundVariableReference scanCtWildcardReference

pvojtechovsky · 2017-11-09T18:00:56Z

every abstract non-leaf CtElement

This might work:

new SpoonMetaModel(getFactory()).getMMTypes().forEach(mmType->{
  if(mmType.getKind()==ABSTRACT && mmType.getModelInteface()!=null)  {
    CtInterface abstractIface = mmType.getModelInteface();
    //abstractIface is called with each abstract interface of spoon model
  }
});

monperrus · 2017-11-09T19:38:46Z

src/test/java/spoon/test/architecture/SpoonArchitectureEnforcerTest.java

+
+ @Test
+ public void testInterfacesAreCtScannable() {
+ // contract: all interface inherited from CtElement should be visited in CtScanner


the contract of this bug should be:
all non-leaf interfaces of the metamodel should be visited by CtInheritance scanner

the interfaces do not have to inherit from ctelement

…r-ctscanner

pvojtechovsky · 2017-11-10T18:46:03Z

src/test/java/spoon/test/architecture/SpoonArchitectureEnforcerTest.java

+ new SpoonMetaModel(interfaces.getFactory()).getMMTypes().forEach(mmType->{
+ if(mmType.getKind()==ABSTRACT && mmType.getModelInterface() != null) {
+ CtInterface abstractIface = mmType.getModelInterface();
+ String methodName = "scan"+abstractIface.getSimpleName();


Thes 2 lines above may be replaced by shorter

String methodName = "scan"+mmType.getName();

monperrus · 2017-11-12T10:35:17Z

@pvojtechovsky would you merge this one?

pvojtechovsky · 2017-11-12T12:46:49Z

src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java

@@ -567,7 +567,7 @@ public void visitCtCatch(CtCatch catchBlock) {
 elementPrinterHelper.writeComment(catchBlock, CommentOffset.BEFORE);
 printer.writeSpace().writeKeyword("catch").writeSpace().writeSeparator("(");
 CtCatchVariable<? extends Throwable> parameter = catchBlock.getParameter();
- if (parameter.getMultiTypes().size() > 1) {
+ if (parameter != null && parameter.getMultiTypes() != null && parameter.getMultiTypes().size() > 1) {


parameter.getMultiTypes() is never null. I suggest to remove this check.
WDYT?

pvojtechovsky · 2017-11-12T13:06:56Z

src/test/java/spoon/test/architecture/SpoonArchitectureEnforcerTest.java

+ interfaces.addInputResource("src/main/java/spoon/reflect/code");
+ interfaces.addInputResource("src/main/java/spoon/reflect/reference");
+ interfaces.addInputResource("src/main/java/spoon/reflect/visitor/CtScanner.java");
+ interfaces.buildModel();


SpoonMetaModel needs these packages too
"spoon/support/reflect/declaration",
"spoon/support/reflect/code",
"spoon/support/reflect/reference"
Otherwise meta model is not complete.

pvojtechovsky · 2017-11-12T13:10:15Z

src/test/java/spoon/test/architecture/SpoonArchitectureEnforcerTest.java

@@ -212,6 +216,34 @@ public boolean matches(CtClass element) {
 }

 @Test
+ public void testInterfacesAreCtScannable() {
+ // contract: all non-leaf interfaces of the metamodel should be visited by CtInheritanceScanner


This test checks that there is a scan method. It is good test. But there is not checked that visitXxx method calls all scan methods for each interface implemented by that non abstract class. I suggest to add this test too. May be as part of another PR?
WDYT?

I thought it was the goal of

spoon/src/test/java/spoon/reflect/visitor/CtInheritanceScannerTest.java

Line 118 in 65c1b29

public void testCtInheritanceScanner() throws Throwable {

pvojtechovsky · 2017-11-12T13:17:09Z

Thank You Simon for that work!
I do not persist on fixing of my suggestions. They are optional. Just let me know if you are done. Then this PR is OK for me.

pvojtechovsky · 2017-11-12T13:42:26Z

src/test/java/spoon/test/architecture/SpoonArchitectureEnforcerTest.java

+
+ List<String> missingMethods = new ArrayList<>();
+
+ new SpoonMetaModel(interfaces.getFactory()).getMMTypes().forEach(mmType -> {


This call will fail after #1714 is merged. Add
"spoon/support/reflect/declaration",
"spoon/support/reflect/code",
"spoon/support/reflect/reference"
packages as resources above to fix it.

pvojtechovsky · 2017-11-13T09:16:43Z

there is not checked that visitXxx method calls all scan methods

spoon/src/test/java/spoon/reflect/visitor/CtInheritanceScannerTest.java

Line 118 in 65c1b29

public void testCtInheritanceScanner() throws Throwable {

You are right, so there is already a test for that.

…r-ctscanner

surli added 3 commits November 9, 2017 14:07

Add a new test to check that all interfaces inherited from CtElement …

a4e3612

…should be visited in CtScanner

Fix the test

1df57af

Improve test with all subtype of CtElement

39ae439

monperrus reviewed Nov 9, 2017

View reviewed changes

Try using Metamodel for the test

f0b7698

surli changed the title ~~Wip refactor: All interfaces inherited from CtElement should be visited in CtScanner~~ Wip refactor: all non-leaf interfaces of the metamodel should be visited by CtInheritanceScanner Nov 10, 2017

surli added 6 commits November 10, 2017 10:47

Merge remote-tracking branch 'inria/master' into add-enforcer-test-fo…

8e88a09

…r-ctscanner

Improve test

bb1ea21

Change MMType

dcd3e03

Fix typoe in RoleHandlerGenerator

9cbdf9d

Change a bit MMType and fix test

8990c82

Fix the test

1a07699

surli changed the title ~~Wip refactor: all non-leaf interfaces of the metamodel should be visited by CtInheritanceScanner~~ review refactor: all non-leaf interfaces of the metamodel should be visited by CtInheritanceScanner Nov 10, 2017

Merge remote-tracking branch 'inria/master' into add-enforcer-test-fo…

75e8812

…r-ctscanner

surli changed the title ~~review refactor: all non-leaf interfaces of the metamodel should be visited by CtInheritanceScanner~~ WiP refactor: all non-leaf interfaces of the metamodel should be visited by CtInheritanceScanner Nov 10, 2017

surli added 4 commits November 10, 2017 11:41

Start to implement new scan methods

33b8524

Fix CtInheritanceScanner test

3f85b08

Fix CloneVisitorGenerator

ffd583d

Merge remote-tracking branch 'inria/master' into add-enforcer-test-fo…

0c44575

…r-ctscanner

surli changed the title ~~WiP refactor: all non-leaf interfaces of the metamodel should be visited by CtInheritanceScanner~~ review refactor: all non-leaf interfaces of the metamodel should be visited by CtInheritanceScanner Nov 10, 2017

pvojtechovsky reviewed Nov 10, 2017

View reviewed changes

pvojtechovsky reviewed Nov 12, 2017

View reviewed changes

Changes after Pavel's review

3d803d4

pvojtechovsky approved these changes Nov 13, 2017

View reviewed changes

surli added 2 commits November 13, 2017 12:47

Merge remote-tracking branch 'inria/master' into add-enforcer-test-fo…

b720fef

…r-ctscanner

Merge remote-tracking branch 'inria/master' into add-enforcer-test-fo…

fa8c088

…r-ctscanner

pvojtechovsky merged commit b62d56e into INRIA:master Nov 13, 2017

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

review refactor: all non-leaf interfaces of the metamodel should be visited by CtInheritanceScanner #1703

review refactor: all non-leaf interfaces of the metamodel should be visited by CtInheritanceScanner #1703

surli commented Nov 9, 2017

surli commented Nov 9, 2017

pvojtechovsky commented Nov 9, 2017

monperrus Nov 9, 2017

pvojtechovsky Nov 10, 2017

monperrus commented Nov 12, 2017

pvojtechovsky Nov 12, 2017

pvojtechovsky Nov 12, 2017

pvojtechovsky Nov 12, 2017

surli Nov 13, 2017

pvojtechovsky commented Nov 12, 2017

pvojtechovsky Nov 12, 2017

pvojtechovsky commented Nov 13, 2017


		List<String> missingMethods = new ArrayList<>();

		new SpoonMetaModel(interfaces.getFactory()).getMMTypes().forEach(mmType -> {

review refactor: all non-leaf interfaces of the metamodel should be visited by CtInheritanceScanner #1703

review refactor: all non-leaf interfaces of the metamodel should be visited by CtInheritanceScanner #1703

Conversation

surli commented Nov 9, 2017

surli commented Nov 9, 2017

pvojtechovsky commented Nov 9, 2017

monperrus Nov 9, 2017

Choose a reason for hiding this comment

pvojtechovsky Nov 10, 2017

Choose a reason for hiding this comment

monperrus commented Nov 12, 2017

pvojtechovsky Nov 12, 2017

Choose a reason for hiding this comment

pvojtechovsky Nov 12, 2017

Choose a reason for hiding this comment

pvojtechovsky Nov 12, 2017

Choose a reason for hiding this comment

surli Nov 13, 2017

Choose a reason for hiding this comment

pvojtechovsky commented Nov 12, 2017

pvojtechovsky Nov 12, 2017

Choose a reason for hiding this comment

pvojtechovsky commented Nov 13, 2017