review: fix replacing of field access by relaxed Parameter value contract

doc/template_definition.md

@@ -209,7 +209,7 @@ is transformed into:
 #### Literal template Parameters
 
 For literals, Spoon provides developers with  *literal template parameters*. When the parameter is known to
-be a literal (primitive types, `String`, `Class` or a one-dimensional array of
+be a literal (primitive types, `Class` or a one-dimensional array of
 these types), a template parameter, annotated with `@Parameter` enables one to have concise template code.
 
 ```java
@@ -222,6 +222,48 @@ val = 5;
 if (list.size()>val) {...}
 ```
 
+String parameters are not working like other primitive type parameters, since we're using String parameters only to rename elements of the code like fields and methods.
+
+```java
+// with String template parameter, which is used to substitute method name. 
+@Parameter
+String methodName;
+...
+methodName = "generatedMethod";
+...
+void methodName() {
+	//a body of generated method
+}
+```
+
+To use a parameter with a type String like other primitive types, use CtLiteral<String>.
+
+```java
+// with CtLiteral<String> template parameter, which is used to substitute String literal 
+@Parameter
+CtLiteral<String> val;
+...
+val = factory.Code().createLiteral("Some string");
+...
+String someMethod() {
+	return val.S();	//is substituted as return "Some string";
+}
+```
+
+or String literal can be optionally generated like this
+
+```java
+// with CtLiteral<String> template parameter, which is used to substitute String literal 
+@Parameter
+String val;
+...
+val = "Some string";
+...
+String someMethod() {
+	return "val";	//is substituted as return "Some string";
+}
+```
+
 Note that AST elements can also be given as parameter using `@Parameter` ([javadoc](http://spoon.gforge.inria.fr/mvnsites/spoon-core/apidocs/spoon/template/Parameter.html))
 annotation.
 
@@ -230,6 +272,6 @@ class ATemplate extends BlockTemplate {
 @Parameter
 CtExpression<String> exp;
 ...
-if ("er".equals(val)) {...}
+if ("er".equals(exp.S())) {...}
 }
 ```

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

@@ -787,6 +787,9 @@ private <T> void printCtFieldAccess(CtFieldAccess<T> f) {
 						 * Search for potential variable declaration until we found a class which declares or inherits this field
 						 */
 						final CtField<?> field = f.getVariable().getFieldDeclaration();
+						if (field == null) {
+							throw new SpoonException("The reference to field named \"" + f.getVariable().getSimpleName() + "\" is invalid, because there is no field with such name on path:" + getPath(f));
+						}
 						final String fieldName = field.getSimpleName();
 						CtVariable<?> var = f.getVariable().map(new PotentialVariableDeclarationFunction(fieldName)).first();
 						if (var != field) {

src/main/java/spoon/support/template/SubstitutionVisitor.java

@@ -243,11 +243,46 @@ private <T> void visitFieldAccess(final CtFieldAccess<T> fieldAccess) {
 						throw context.replace(toReplace, enumValueAccess);
 					} else if ((value != null) && value.getClass().isArray()) {
 						throw context.replace(toReplace, factory.Code().createLiteralArray((Object[]) value));
+					} else if (fieldAccess == toReplace && value instanceof String) {
+						/*
+						 * If the value is type String, then it is ambiguous request, because:
+						 * A) sometime client wants to replace parameter field access by String literal
+						 *
+						 * @Parameter
+						 * String field = "x"
+						 *
+						 * System.printLn(field) //is substitutes as: System.printLn("x")
+						 *
+						 * but in the case of local variables it already behaves like this
+						 * {
+						 * 		int field;
+						 * 		System.printLn(field) //is substitutes as: System.printLn(x)
+						 * }
+						 *
+						 * B) sometime client wants to keep field access and just substitute field name
+						 *
+						 * @Parameter("field")
+						 * String fieldName = "x"
+						 *
+						 * System.printLn(field) //is substitutes as: System.printLn(x)
+						 *
+						 * ----------------------
+						 *
+						 * The case B is more clear and is compatible with substitution of name of local variable, method name, etc.
+						 * And case A can be easily modeled using this clear code
+						 *
+						 * @Parameter
+						 * String field = "x"
+						 * System.printLn("field") //is substitutes as: System.printLn("x")
+						 * System.printLn(field) //is substitutes as: System.printLn("x") because the parameter `field` is constructed with literal value
+						 */
+						//New implementation always replaces the name of the accessed field
+						//so do nothing here. The string substitution is handled by #scanCtReference
 					} else {
 						throw context.replace(toReplace, factory.Code().createLiteral(value));
 					}
 				} else {
-					throw context.replace(toReplace, toReplace.clone());
+					throw context.replace(toReplace, (CtElement) value);
 				}
 			}
 		}
@@ -501,14 +536,14 @@ private static <T extends CtElement> T getFirst(CtElement ele, Class<T> clazz) {
 	 * @return list where each item is assured to be of type itemClass
 	 */
 	@SuppressWarnings("unchecked")
-	private static <T> List<T> getParameterValueAsListOfClones(Class<T> itemClass, Object parameterValue) {
+	private <T> List<T> getParameterValueAsListOfClones(Class<T> itemClass, Object parameterValue) {
 		List<Object> list = getParameterValueAsNewList(parameterValue);
 		for (int i = 0; i < list.size(); i++) {
 			list.set(i, getParameterValueAsClass(itemClass, list.get(i)));
 		}
 		return (List<T>) list;
 	}
-	private static List<Object> getParameterValueAsNewList(Object parameterValue) {
+	private List<Object> getParameterValueAsNewList(Object parameterValue) {
 		List<Object> list = new ArrayList<>();
 		if (parameterValue != null) {
 			if (parameterValue instanceof Object[]) {
@@ -536,7 +571,7 @@ private static List<Object> getParameterValueAsNewList(Object parameterValue) {
 	 * @return parameterValue cast (in future potentially converted) to itemClass
 	 */
 	@SuppressWarnings("unchecked")
-	private static <T> T getParameterValueAsClass(Class<T> itemClass, Object parameterValue) {
+	private <T> T getParameterValueAsClass(Class<T> itemClass, Object parameterValue) {
 		if (parameterValue == null || parameterValue == NULL_VALUE) {
 			return null;
 		}
@@ -556,7 +591,11 @@ private static <T> T getParameterValueAsClass(Class<T> itemClass, Object paramet
 			if (parameterValue instanceof CtTypeReference) {
 				//convert type reference into code element as class access
 				CtTypeReference<?> tr = (CtTypeReference<?>) parameterValue;
-				return (T) tr.getFactory().Code().createClassAccess(tr);
+				return (T) factory.Code().createClassAccess(tr);
+			}
+			if (parameterValue instanceof String) {
+				//convert String to code element as Literal
+				return (T) factory.Code().createLiteral((String) parameterValue);
 			}
 		}
 		throw new SpoonException("Parameter value has unexpected class: " + parameterValue.getClass().getName() + ". Expected class is: " + itemClass.getName());
@@ -565,7 +604,7 @@ private static <T> T getParameterValueAsClass(Class<T> itemClass, Object paramet
 	 * @param parameterValue a value of an template parameter
 	 * @return parameter value converted to String
 	 */
-	private static String getParameterValueAsString(Object parameterValue) {
+	private String getParameterValueAsString(Object parameterValue) {
 		if (parameterValue == null) {
 			return null;
 		}
@@ -634,7 +673,7 @@ private static <T> CtTypeReference<T> getParameterValueAsTypeReference(Factory f
 	 * @param index index of item from the list, or null if item is not expected to be a list
 	 * @return parameterValue (optionally item from the list) cast (in future potentially converted) to itemClass
 	 */
-	private static <T> T getParameterValueAtIndex(Class<T> itemClass, Object parameterValue, Integer index) {
+	private <T> T getParameterValueAtIndex(Class<T> itemClass, Object parameterValue, Integer index) {
 		if (index != null) {
 			//convert to list, but do not clone
 			List<Object> list = getParameterValueAsNewList(parameterValue);

src/main/java/spoon/template/Template.java

@@ -26,9 +26,10 @@
  * {@link TemplateParameter#S()} method.
  *
  * <p>
- * When the template parameter is a String or a primitive type (or a boxing
+ * When the template parameter is a String it is used to rename element of the code such as fields or methods.
+ * When it is another primitive type (or a boxing
  * type) representing a literal, or a Class, the template parameter can be
- * directly accessed.
+ * directly accessed. To use a standard parameter containing a String type, use a CtLiteral&lt;String&gt;
  *
  * <pre>
  *       import spoon.template.Template;
@@ -38,15 +39,19 @@
  *           // template parameter fields
  *            \@Parameter String _parameter_;
  *
+ *            \@Parameter CtLiteral&lt;String&gt; _anotherParameter;
+ *
+ *
  *           // parameters binding
  *            \@Local
- *           public SimpleTemplate(String parameter) {
+ *           public SimpleTemplate(String parameter, CtLiteral&lt;String&gt; anotherParameter) {
  *               _parameter_ = parameter;
+ *               _anotherParameter = anotherParameter;
  *           }
  *
  *           // template method
- *           public void simpleTemplateMethod() {
- *               System.out.println(_parameter_);
+ *           public void methodwith_parameter_() {
+ *               System.out.println(_anotherParameter);
  *           }
  *       }
  * </pre>
@@ -60,7 +65,10 @@
  *
  * <pre>
  *       spoon.reflect.CtClass target=...;
- *       Template template=new SimpleTemplate(&quot;hello templated world&quot;);
+ *       CtLiteral&lt;String&gt; anotherParameter = factory.createLiteral();
+ *       anotherParameter.setValue(&quot;hello templated world&quot;);
+ *
+ *       Template template=new SimpleTemplate(&quot;ParameterizedName&quot;, anotherParameter);
  *       Substitution.insertAll(target,template);
  * </pre>
  *
@@ -70,7 +78,7 @@
  *
  * <pre>
  * public class A {
- * 	public void insertedMethod() {
+ * 	public void methodwithParameterizedName() {
  * 		System.out.println(&quot;hello templated world&quot;);
  *    }
  * }

src/test/java/spoon/test/prettyprinter/PrinterTest.java

@@ -157,7 +157,6 @@ public void testPrintingOfOrphanFieldReference() throws Exception {
 			type.getMethodsByName("failingMethod").get(0).getBody().getStatement(0).toString();
 			fail();
 		} catch (SpoonException e) {
-			assertTrue(e.getCause() instanceof NullPointerException);
 			//the name of the missing field declaration is part of exception
 			assertTrue(e.getMessage().indexOf("testedField")>=0);
 			//the name of the method where field declaration is missing is part of exception

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

@@ -30,7 +30,10 @@
 import spoon.template.Substitution;
 import spoon.template.TemplateMatcher;
 import spoon.template.TemplateParameter;
+import spoon.test.template.testclasses.AnotherFieldAccessTemplate;
 import spoon.test.template.testclasses.ArrayAccessTemplate;
+import spoon.test.template.testclasses.FieldAccessOfInnerClassTemplate;
+import spoon.test.template.testclasses.FieldAccessTemplate;
 import spoon.test.template.testclasses.InnerClassTemplate;
 import spoon.test.template.testclasses.InvocationTemplate;
 import spoon.test.template.testclasses.LoggerModel;
@@ -48,6 +51,7 @@
 import spoon.test.template.testclasses.constructors.C1;
 import spoon.test.template.testclasses.constructors.TemplateWithConstructor;
 import spoon.test.template.testclasses.constructors.TemplateWithFieldsAndMethods;
+import spoon.test.template.testclasses.constructors.TemplateWithFieldsAndMethods_Wrong;
 import spoon.test.template.testclasses.inheritance.InterfaceTemplate;
 import spoon.test.template.testclasses.inheritance.SubClass;
 import spoon.test.template.testclasses.inheritance.SubTemplate;
@@ -190,6 +194,25 @@ public void testTemplateInheritance() throws Exception {
 		//contract: for each whose expression is not a template parameter is not inlined
 		assertTrue(methodWithTemplatedParameters.getBody().getStatement(11) instanceof CtForEach);
 
+		// contract: local variable write are replaced by local variable write with modified local variable name
+		assertEquals("newVarName = o", methodWithTemplatedParameters.getBody().getStatement(12).toString());
+
+		// contract: local variable read are replaced by local variable read with modified local variable name
+		assertEquals("l = ((java.util.LinkedList) (newVarName))", methodWithTemplatedParameters.getBody().getStatement(13).toString());
+
+		// contract; field access is handled same like local variable access
+		CtMethod<?> methodWithFieldAccess = subc.getElements(
+				new NameFilter<CtMethod<?>>("methodWithFieldAccess")).get(0);
+		elementToGeneratedByMember.put(methodWithFieldAccess, "#methodWithFieldAccess");
+		elementToGeneratedByMember.put(subc.getField("newVarName"), "#var");
+
+		// contract: field write are replaced by field write with modified field name
+		assertEquals("newVarName = o", methodWithFieldAccess.getBody().getStatement(2).toString());
+
+		// contract: field read are replaced by field read with modified field name
+		assertEquals("l = ((java.util.LinkedList) (newVarName))", methodWithFieldAccess.getBody().getStatement(3).toString());
+
+
 		class Context {
 			int nrTypeMembers = 0;
 			int nrOthers = 0;
@@ -318,7 +341,34 @@ public void testTemplateC1() throws Exception {
 
 		assertEquals(0, factory.getEnvironment().getErrorCount());
 		assertEquals(0, factory.getEnvironment().getWarningCount());
+	}
+
+	@Test
+	public void testTemplateWithWrongUsedStringParam() throws Exception {
+		Launcher spoon = new Launcher();
+		Factory factory = spoon.createFactory();
+		spoon.createCompiler(
+				factory,
+				SpoonResourceHelper
+						.resources("./src/test/java/spoon/test/template/testclasses/constructors/C1.java"),
+				SpoonResourceHelper
+						.resources(
+								"./src/test/java/spoon/test/template/testclasses/constructors/TemplateWithFieldsAndMethods_Wrong.java"))
+				.build();
+
+		CtClass<?> c1 = factory.Class().get(C1.class);
+
+		new TemplateWithFieldsAndMethods_Wrong(
+				"testparam").apply(c1);
 
+		CtMethod<?> m = c1.getMethod("methodToBeInserted");
+		assertNotNull(m);
+		//contract: printing of code which contains invalid field reference, fails with nice exception
+		try {
+			m.getBody().getStatement(0).toString();
+		} catch (SpoonException e) {
+			assertTrue("The error description doesn't contain name of invalid field. There is:\n" + e.getMessage(), e.getMessage().indexOf("testparam") >= 0);
+		}
 	}
 
 	@Test
@@ -505,8 +555,8 @@ public void testExtensionDecoupledSubstitutionVisitor() throws Exception {
 
 
 		Map<String, Object> params = new HashMap<>();
-		params.put("_classname_", aTargetType.getSimpleName()) ;
-		params.put("_methodName_", toBeLoggedMethod.getSimpleName());
+		params.put("_classname_", factory.Code().createLiteral(aTargetType.getSimpleName()));
+		params.put("_methodName_", factory.Code().createLiteral(toBeLoggedMethod.getSimpleName()));
 		params.put("_block_", toBeLoggedMethod.getBody());
 		final List<CtMethod<?>> aMethods = new SubstitutionVisitor(factory, params).substitute(aTemplateModel.clone());
 		assertEquals(1, aMethods.size());
@@ -908,4 +958,60 @@ public void testObjectIsNotParamTemplate() throws Exception {
 		assertEquals(0, result.getMethodsByName("methXXXd").size());
 		assertEquals(1, result.getMethodsByName("method").size());
 	}
+
+	@Test
+	public void testFieldAccessNameSubstitution() throws Exception {
+		//contract: the substitution of name of whole field is possible
+		Launcher spoon = new Launcher();
+		spoon.addTemplateResource(new FileSystemFile("./src/test/java/spoon/test/template/testclasses/FieldAccessTemplate.java"));
+
+		spoon.buildModel();
+		Factory factory = spoon.getFactory();
+
+		{
+			//contract: String value is substituted in String literal
+			final CtClass<?> result = (CtClass<?>) new FieldAccessTemplate("value").apply(factory.Class().create("x.X"));
+			assertEquals("int value;", result.getField("value").toString());
+
+			assertEquals("value = 7", result.getMethodsByName("m").get(0).getBody().getStatement(0).toString());
+		}
+	}
+
+	@Test
+	public void testFieldAccessNameSubstitutionInInnerClass() throws Exception {
+		//contract: the substitution of name of whole field is possible in inner class too
+		Launcher spoon = new Launcher();
+		spoon.addTemplateResource(new FileSystemFile("./src/test/java/spoon/test/template/testclasses/FieldAccessOfInnerClassTemplate.java"));
+
+		spoon.buildModel();
+		Factory factory = spoon.getFactory();
+
+		{
+			//contract: String value is substituted in String literal
+			final CtClass<?> result = (CtClass<?>) new FieldAccessOfInnerClassTemplate("value").apply(factory.Class().create("x.X"));
+			final CtClass<?> innerClass = result.getNestedType("Inner");
+			assertEquals("int value;", innerClass.getField("value").toString());
+
+			assertEquals("value = 7", innerClass.getMethodsByName("m").get(0).getBody().getStatement(0).toString());
+		}
+	}
+
+	@Test
+	public void testAnotherFieldAccessNameSubstitution() throws Exception {
+		//contract: the substitution of name of whole field is possible
+		Launcher spoon = new Launcher();
+		spoon.addTemplateResource(new FileSystemFile("./src/test/java/spoon/test/template/testclasses/AnotherFieldAccessTemplate.java"));
+
+		spoon.buildModel();
+		Factory factory = spoon.getFactory();
+
+		{
+			//contract: String value is substituted in String literal
+			final CtClass<?> result = (CtClass<?>) new AnotherFieldAccessTemplate().apply(factory.Class().create("x.X"));
+			assertEquals("int x;", result.getField("x").toString());
+			assertEquals("int m_x;", result.getField("m_x").toString());
+
+			assertEquals("java.lang.System.out.println(((x) + (m_x)))", result.getAnonymousExecutables().get(0).getBody().getStatement(0).toString());
+		}
+	}
 }