review: refactor SubstitutionVisitor #1359

pvojtechovsky · 2017-06-04T20:06:57Z

Here is refactored SubstitutionVisitor. There are following changes:

C1) The SubstitutionVisitor can be used on any Spoon model and Map<String, Object> parameters. It does not need Template, TemplateParameter, Parameter. Of course they may be still used if it makes sense for client. See new constructor SubstitutionVisitor(Factory f, Map<String, Object> templateParameters)

C2) The SubstitutionVisitor can substitute the root node of substitution model by zero, one or more nodes.
See new method <E extends CtElement> List<E> SubstitutionVisitor#substitute(E element)

C3) There is possible to directly generate new Class, Interface, Enum, ... thanks to (C1). See
CtType<T> Substitution#insertType(CtPackage targetPackage, String typeName, CtType<T> templateOfType, Map<String, Object> templateParameters)

C4) The SubstitutionVisitor use Context, which knows current substitution parameters. It is needed to correctly substitute all parameters of blocks generated by SubstitutionVisitor#visitCtForEach

C5) The SubstitutionVisitor has less scanning methods now. It does all substitutions here:

scanCtElement - substitutes comments, which are on all elements
scanCtNamedElement - substitutes names or part's of names. Or replaces whole nodes by zero, one or more other nodes. For example legacy CtExecutable one parameter replacement by many parameters, is implemented this more generic way. Now we can replace everything this way. For example: one Annotation by many annotations; one field by many fields; one enum value many enum values, ...
scanCtReference - substitutes all kinds of references. See code for each case
visitCtForEach - like before
visitCtFieldRead, visitCtFieldWrite - like before
visitCtInvocation - like before

C6) the package protected fields of SubstitutionVisitor were made private - may be it was public intentional? Then I will rollback this change

C7) the conversion of parameter values to required types were extracted to private methods and made more reliable. Missuses throws exception, and if conversion is possible then parameter value is automatically converted to required type. See SubstitutionVisitor#getParameterValueAsXxx methods.

monperrus · 2017-06-12T07:50:20Z

rebase?

pvojtechovsky · 2017-06-12T20:37:23Z

I have added some more tests to increase coverage of the new implementation of SubstitutionVisitor.
This PR is ready for review. I know that it is not baby step, but it is hard to split it, because it depends on each other ...

spoon-bot · 2017-06-13T19:59:23Z

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.8.0-20170612.224545-39

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

Detected changes: 2.

Change 1

Name	Element
Old	class spoon.support.template.SubstitutionVisitor.InheritanceSustitutionScanner
New	class spoon.support.template.SubstitutionVisitor.InheritanceSustitutionScanner
Code	java.class.visibilityReduced
Description	Visibility was reduced from 'public' to 'private'.
Breaking	binary: breaking,

Change 2

Name	Element
Old	method void spoon.support.template.SubstitutionVisitor.InheritanceSustitutionScanner::(spoon.support.template.SubstitutionVisitor)
New	method void spoon.support.template.SubstitutionVisitor.InheritanceSustitutionScanner::()
Code	java.method.visibilityReduced
Description	visibility reduced
Breaking	binary: breaking,

monperrus

This is very nice, thanks. See my comments.

monperrus · 2017-06-15T06:09:50Z

src/test/java/spoon/test/template/TemplateInvocationSubstitutionTest.java

+		spoon.buildModel();
+		Factory factory = spoon.getFactory();
+
+		CtBlock<?> model = factory.Class().get(InvocationSubstitutionByStatementTemplate.class).getMethod("sample").getBody();


model confusing here what about templateArg

monperrus · 2017-06-15T06:10:00Z

src/test/java/spoon/test/template/TemplateArrayAccessTest.java

 	}

+	@Test
+	public void testArrayLengthAccess() throws Exception {
+		//contract: the template engine replaces length of collection of parameter values by number


monperrus · 2017-06-15T06:11:05Z

src/test/java/spoon/test/template/TemplateInvocationSubstitutionTest.java

+
+	@Test
+	public void testInvocationSubstitutionByExpression() throws Exception {
+		//contract: the template engine supports substitution of any method invocation


//contract: the template engine supports substitution of method arguments

it is misunderstanding. The origin contract is correct.

System.out.println(_expression_().substring(1));

is substituted by

System.out.println("abc".substring(1));

so not the method arguments, but any method invocation can be substituted.

monperrus · 2017-06-15T06:14:07Z

src/test/java/spoon/test/template/TemplateTest.java

+
+	@Test
+	public void testStatementTemplateRootSubstitution() throws Exception {
+		//contract: the template engine supports substitution of root element


I'm sure to understand. there is no "root" in this test

SubstituteRootTemplate extends StatementTemplate, which internally calls

CtStatement result = c.getMethod("statement").getBody().getStatements().get(0).clone();

where result is block.S(); ... so it is substitution of the root element in the spoon model represented by result.

monperrus · 2017-06-15T06:17:05Z

src/test/java/spoon/test/template/TemplateTest.java

+
+	@Test
+	public void testInsertType() throws Exception {
+		final Launcher launcher = new Launcher();


I'd rename the method createTypeFromTemplate

contract: the Substituion API provides a method createTypeFromTemplate

the Substitution.insertType was renamed to createTypeFromTemplate

monperrus · 2017-06-15T06:17:41Z

src/test/java/spoon/test/template/TemplateTest.java

+
+		//contract: we can generate interface
+		final CtType<?> aIfaceModel = launcher.getFactory().Interface().get(AnIfaceModel.class);
+		CtType<?> genIface = Substitution.insertType(targetPackage, "GenIface", aIfaceModel, parameters);


What about only passing a String qualifiedName to simplify the API usage?

old insertType renamed to createTypeFromTemplate has now only String qualifiedName instead of CtPackage and simpleName.

pvojtechovsky · 2017-06-15T18:35:30Z

@monperrus Thanks for ideas and your time to review this bigger PR.
What next?

spoon-bot · 2017-06-15T18:39:03Z

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.8.0-20170614.224555-42

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

Detected changes: 2.

Change 1

Name	Element
Old	class spoon.support.template.SubstitutionVisitor.InheritanceSustitutionScanner
New	class spoon.support.template.SubstitutionVisitor.InheritanceSustitutionScanner
Code	java.class.visibilityReduced
Description	Visibility was reduced from 'public' to 'private'.
Breaking	binary: breaking,

Change 2

Name	Element
Old	method void spoon.support.template.SubstitutionVisitor.InheritanceSustitutionScanner::(spoon.support.template.SubstitutionVisitor)
New	method void spoon.support.template.SubstitutionVisitor.InheritanceSustitutionScanner::()
Code	java.method.visibilityReduced
Description	visibility reduced
Breaking	binary: breaking,

monperrus · 2017-06-19T11:53:34Z

OK to merge.

pvojtechovsky force-pushed the simplifySubstVisitor branch 2 times, most recently from 3269ae8 to ebfbba9 Compare June 8, 2017 20:40

pvojtechovsky changed the title ~~WiP: remove useless code from SubstitutionVisitor~~ WiP: refactor SubstitutionVisitor Jun 8, 2017

pvojtechovsky force-pushed the simplifySubstVisitor branch 3 times, most recently from fe0689c to 3f3c20c Compare June 11, 2017 20:27

pvojtechovsky changed the title ~~WiP: refactor SubstitutionVisitor~~ review2: refactor SubstitutionVisitor Jun 11, 2017

monperrus changed the title ~~review2: refactor SubstitutionVisitor~~ WIP: refactor SubstitutionVisitor Jun 12, 2017

pvojtechovsky force-pushed the simplifySubstVisitor branch from 3f3c20c to 10aca7c Compare June 12, 2017 08:09

pvojtechovsky added 2 commits June 12, 2017 21:03

refactor SubstitutionVisitor

9afb50f

test SubstitutionVisitor

2ba4638

pvojtechovsky added 3 commits June 12, 2017 22:20

test forEach not/inlining and blocks

951fd9c

test array.length substituted to number literal

1b0ba15

test substitution by missing value

2eaef6c

pvojtechovsky force-pushed the simplifySubstVisitor branch from 10aca7c to 2eaef6c Compare June 12, 2017 20:33

INRIA deleted a comment from spoon-bot Jun 13, 2017

surli changed the title ~~WIP: refactor SubstitutionVisitor~~ review: refactor SubstitutionVisitor Jun 13, 2017

pvojtechovsky added 5 commits June 13, 2017 20:35

CtBlock.body is always CtBlock

38367bc

improve test

6773854

fix: handling of null parameter values

829d6b9

test null values in parameters

c4bc1cd

feature: Class is converted to String

e892ca2

pvojtechovsky added 4 commits June 13, 2017 21:29

test Class to String and test substitution of method name and comments

aa09970

feature: substitute all comments (not only JavaDoc)

0a09a9a

feature: convert CtInvocation, CtExecutable(Reference) to String

9ef4d57

test substitution of all comment types and invocation in comment

c40b814

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

monperrus reviewed Jun 15, 2017

View reviewed changes

pvojtechovsky added 2 commits June 15, 2017 20:30

rename local variable model to templateArg

ef04b27

insertType(CtPackage, String, ...)->createTypeFromTemplate(String qname)

b8a5947

surli merged commit 293d8e8 into INRIA:master Jun 19, 2017

pvojtechovsky deleted the simplifySubstVisitor branch June 20, 2017 15:18

surli mentioned this pull request Jul 11, 2017

Spoon Release 5.8.0 Incoming #1470

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

review: refactor SubstitutionVisitor #1359

review: refactor SubstitutionVisitor #1359

pvojtechovsky commented Jun 4, 2017 •

edited

Loading

monperrus commented Jun 12, 2017

pvojtechovsky commented Jun 12, 2017

spoon-bot commented Jun 13, 2017

monperrus left a comment

monperrus Jun 15, 2017

pvojtechovsky Jun 15, 2017

monperrus Jun 15, 2017

monperrus Jun 15, 2017

pvojtechovsky Jun 15, 2017

monperrus Jun 15, 2017

pvojtechovsky Jun 15, 2017

monperrus Jun 15, 2017

pvojtechovsky Jun 15, 2017

monperrus Jun 15, 2017

pvojtechovsky Jun 15, 2017

pvojtechovsky commented Jun 15, 2017

spoon-bot commented Jun 15, 2017

monperrus commented Jun 19, 2017

review: refactor SubstitutionVisitor #1359

review: refactor SubstitutionVisitor #1359

Conversation

pvojtechovsky commented Jun 4, 2017 • edited Loading

monperrus commented Jun 12, 2017

pvojtechovsky commented Jun 12, 2017

spoon-bot commented Jun 13, 2017

Revapi Analysis results

Change 1

Change 2

monperrus left a 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

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

Choose a reason for hiding this comment

pvojtechovsky commented Jun 15, 2017

spoon-bot commented Jun 15, 2017

Revapi Analysis results

Change 1

Change 2

monperrus commented Jun 19, 2017

pvojtechovsky commented Jun 4, 2017 •

edited

Loading