feature: reject invalid simple names (#3189)

src/main/java/spoon/support/reflect/reference/CtReferenceImpl.java

@@ -5,6 +5,7 @@
  */
 package spoon.support.reflect.reference;
 
+import spoon.SpoonException;
 import spoon.reflect.annotations.MetamodelPropertyField;
 import spoon.reflect.code.CtComment;
 import spoon.reflect.declaration.CtElement;
@@ -17,14 +18,18 @@
 
 import java.io.Serializable;
 import java.lang.reflect.AnnotatedElement;
+import java.util.Collection;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Objects;
-
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 import static spoon.reflect.path.CtRole.NAME;
 
 public abstract class CtReferenceImpl extends CtElementImpl implements CtReference, Serializable {
 
 	private static final long serialVersionUID = 1L;
+	private static Collection<String> keywords = fillWithKeywords();
 
 	@MetamodelPropertyField(role = NAME)
 	protected String simplename = "";
@@ -42,6 +47,7 @@ public String getSimpleName() {
 	@Override
 	public <T extends CtReference> T setSimpleName(String simplename) {
 		Factory factory = getFactory();
+		checkIdentiferForJLSCorrectness(simplename);
 		if (factory == null) {
 			this.simplename = simplename;
 			return (T) this;
@@ -54,6 +60,9 @@ public <T extends CtReference> T setSimpleName(String simplename) {
 		return (T) this;
 	}
 
+
+
+
 	@UnsettableProperty
 	@Override
 	public <E extends CtElement> E setComments(List<CtComment> comments) {
@@ -81,4 +90,45 @@ public boolean equals(Object o) {
 		}
 		return false;
 	}
+	private void checkIdentiferForJLSCorrectness(String simplename) {
+	/*
+  * At the level of the Java Virtual Machine, every constructor written in the Java programming language (JLS §8.8)
+  * appears as an instance initialization method that has the special name <init>.
+  * This name is supplied by a compiler. Because the name is not a valid identifier,
+  * it cannot be used directly in a program written in the Java programming language.
+  */
+	//JDTTreeBuilderHelper.computeAnonymousName returns "$numbers$Name" so we have to skip them if they start with numbers
+	//allow empty identifier because they are sometimes used.
+		if (!simplename.matches("<.*>|\\d.*|^.{0}$")) {
+			//split at "<" and ">" because "Iterator<Cache.Entry<K,Store.ValueHolder<V>>>" submits setSimplename ("Cache.Entry<K")
+			String[] splittedSimplename = simplename.split("\\.|<|>");
+			if (checkAllParts(splittedSimplename)) {
+				throw new SpoonException("Not allowed javaletter or keyword in identifier found. See JLS for correct identifier. Identifier: " + simplename);
+			}
+	}
+}
+	private boolean isKeyword(String simplename) {
+		return keywords.contains(simplename);
+	}
+	private boolean checkAllParts(String[] simplenameParts) {
+		for (String simpleName:simplenameParts) {
+			//because arrays use e.g. int[] and @Number is used for instances of an object e.g. foo@1
+			simpleName = simpleName.replaceAll("\\[\\]|@", "");
+			if (isKeyword(simpleName) || checkIdentifierChars(simpleName)) {
+				return true;
+		}
+	}
+	return false;
+	}
+	private boolean checkIdentifierChars(String simplename) {
+		return (!Character.isJavaIdentifierStart(simplename.charAt(0))) || simplename.chars().anyMatch(letter -> !Character.isJavaIdentifierPart(letter));
+	}
+	private static Collection<String> fillWithKeywords() {
+	//removed types because needed as ref: "int","short", "char", "void", "byte","float", "true","false","boolean","double","long","class", "null"
+	return Stream.of("abstract", "continue", "for", "new", "switch", "assert", "default", "if", "package", "synchronized",  "do", "goto", "private",
+	"this", "break",  "implements", "protected", "throw", "else", "import", "public", "throws", "case", "enum", "instanceof", "return",
+	"transient", "catch", "extends", "try", "final", "interface", "static", "finally",  "strictfp", "volatile",
+	"const",  "native", "super", "while", "_")
+	.collect(Collectors.toCollection(HashSet::new));
+	}
 }

src/test/java/spoon/generating/CorrectIdentifierTest.java

@@ -0,0 +1,55 @@
+package spoon.generating;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import org.junit.Ignore;
+import org.junit.Test;
+import spoon.Launcher;
+import spoon.SpoonException;
+import spoon.reflect.reference.CtLocalVariableReference;
+/**
+ * for correct identifier see JLS chapter 3.8 and for keywords 3.9.
+ * Ignored tests because we have to cut some corners between spec and jdt.
+ */
+public class CorrectIdentifierTest {
+
+	@Test
+	public void wrongIdentifer() {
+	CtLocalVariableReference<Object> localVariableRef = new Launcher().getFactory().createLocalVariableReference();
+	assertThrows(SpoonException.class, () -> localVariableRef.setSimpleName("tacos.EatIt()"));
+	}
+	@Test
+	public void wrongIdentifer2() {
+	CtLocalVariableReference<Object> localVariableRef = new Launcher().getFactory().createLocalVariableReference();
+	assertThrows(SpoonException.class, () -> localVariableRef.setSimpleName(";tacos"));
+	}
+	@Ignore
+	@Test
+	public void keyWord() {
+	CtLocalVariableReference<Object> localVariableRef = new Launcher().getFactory().createLocalVariableReference();
+	assertThrows(SpoonException.class, () -> localVariableRef.setSimpleName("class"));
+	}
+	@Ignore
+	@Test
+	public void keyWord2() {
+	CtLocalVariableReference<Object> localVariableRef = new Launcher().getFactory().createLocalVariableReference();
+	assertThrows(SpoonException.class, () -> localVariableRef.setSimpleName("null"));
+	}
+	@Ignore
+	@Test
+	public void keyWord3() {
+	CtLocalVariableReference<Object> localVariableRef = new Launcher().getFactory().createLocalVariableReference();
+	assertThrows(SpoonException.class, () -> localVariableRef.setSimpleName("true"));
+	}
+	@Test
+	public void correctIdentifer() {
+	CtLocalVariableReference<Object> localVariableRef = new Launcher().getFactory().createLocalVariableReference();
+	assertDoesNotThrow(() -> localVariableRef.setSimpleName("EatIt"));
+
+	}
+	@Test
+	public void correctIdentifer2() {
+	CtLocalVariableReference<Object> localVariableRef = new Launcher().getFactory().createLocalVariableReference();
+	assertDoesNotThrow(() -> localVariableRef.setSimpleName("ClassFoo"));
+	}
+}

src/test/java/spoon/test/api/MetamodelTest.java

@@ -520,12 +520,12 @@ public void singleValueRoleAddSetRemove() {
 		//contract: single value roles supports multivalue interface too
 		Launcher launcher = new Launcher();
 		Factory factory = launcher.getFactory();
-		CtTypeReference<?> typeRef = factory.Type().createReference("some.test.package.TestType");
+		CtTypeReference<?> typeRef = factory.Type().createReference("some.test.foo.TestType");
 		RoleHandler rh = RoleHandlerHelper.getRoleHandler(typeRef.getClass(), CtRole.PACKAGE_REF);
 
 		//contract: single value role provides a List
 		List<CtPackageReference> packages = rh.asList(typeRef);
-		assertListContracts(packages, typeRef, 1, "some.test.package");
+		assertListContracts(packages, typeRef, 1, "some.test.foo");
 
 		//contract: adding of existing value fails and changes nothing
 		try {
@@ -534,7 +534,7 @@ public void singleValueRoleAddSetRemove() {
 		} catch (Exception e) {
 			//OK
 		}
-		assertListContracts(packages, typeRef, 1, "some.test.package");
+		assertListContracts(packages, typeRef, 1, "some.test.foo");
 
 		//contract: adding of null fails and changes nothing
 		try {
@@ -543,7 +543,7 @@ public void singleValueRoleAddSetRemove() {
 		} catch (Exception e) {
 			//OK
 		}
-		assertListContracts(packages, typeRef, 1, "some.test.package");
+		assertListContracts(packages, typeRef, 1, "some.test.foo");
 
 		//contract: adding of different value fails, and changes nothing
 		try {
@@ -552,18 +552,18 @@ public void singleValueRoleAddSetRemove() {
 		} catch (SpoonException e) {
 			//OK
 		}
-		assertListContracts(packages, typeRef, 1, "some.test.package");
+		assertListContracts(packages, typeRef, 1, "some.test.foo");
 
 		//contract remove of different value changes nothing
 		assertFalse(packages.remove(factory.Package().createReference("some.test.another_package")));
-		assertListContracts(packages, typeRef, 1, "some.test.package");
+		assertListContracts(packages, typeRef, 1, "some.test.foo");
 
 		//contract remove of null value changes nothing
 		assertFalse(packages.remove(null));
-		assertListContracts(packages, typeRef, 1, "some.test.package");
+		assertListContracts(packages, typeRef, 1, "some.test.foo");
 
 		//contract remove of existing value sets value to null and size to 0
-		assertTrue(packages.remove(factory.Package().createReference("some.test.package")));
+		assertTrue(packages.remove(factory.Package().createReference("some.test.foo")));
 		assertListContracts(packages, typeRef, 0, null);
 
 		//contract add of null into empty collection changes size to 1, but value is still null
@@ -580,11 +580,11 @@ public void singleValueRoleAddSetRemove() {
 		assertListContracts(packages, typeRef, 1, null);
 
 		//contract: set of new value replaces existing value
-		assertNull(packages.set(0, factory.Package().createReference("some.test.package")));
-		assertListContracts(packages, typeRef, 1, "some.test.package");
+		assertNull(packages.set(0, factory.Package().createReference("some.test.foo")));
+		assertListContracts(packages, typeRef, 1, "some.test.foo");
 
 		//contract: set of null value keeps size==1 even if value is replaced by null
-		assertEquals("some.test.package", packages.set(0, null).getQualifiedName());
+		assertEquals("some.test.foo", packages.set(0, null).getQualifiedName());
 		assertListContracts(packages, typeRef, 1, null);
 
 		//contract: remove of null value by index sets size==0 the value is still null

src/test/java/spoon/test/arrays/ArraysTest.java

@@ -73,7 +73,7 @@ public void testArrayReferences() throws Exception {
 		assertTrue(ctType.getSimpleName().contains("[]"));
 
 		// solution 3:  use isSubtypeOf
-        	assertTrue(x.getType().isSubtypeOf(x.getFactory().Type().get(Array.class).getReference()));
+		assertTrue(x.getType().isSubtypeOf(x.getFactory().Type().get(Array.class).getReference()));
 
 		// solution 4: you can ask for actual class and then the component type if any
 		assertTrue(typeRef.getActualClass().getComponentType() != null);

src/test/java/spoon/test/compilationunit/TestCompilationUnit.java

@@ -100,11 +100,11 @@ public void testGetUnitTypeWorksWithDeclaredPackage() {
 	@Test
 	public void testGetUnitTypeWorksWithCreatedObjects() {
 		final Launcher launcher = new Launcher();
-		CtPackage myPackage = launcher.getFactory().Package().getOrCreate("my.package");
+		CtPackage myFooPackage = launcher.getFactory().Package().getOrCreate("my.foo");
 		CompilationUnit cu = launcher.getFactory().createCompilationUnit();
 		assertEquals(CompilationUnit.UNIT_TYPE.UNKNOWN, cu.getUnitType());
 
-		cu.setDeclaredPackage(myPackage);
+		cu.setDeclaredPackage(myFooPackage);
 		assertEquals(CompilationUnit.UNIT_TYPE.PACKAGE_DECLARATION, cu.getUnitType());
 
 		cu.setDeclaredTypes(Collections.singletonList(launcher.getFactory().createClass()));
@@ -183,18 +183,18 @@ public void testNewlyCreatedCUWouldGetAPartialPosition() throws IOException {
 		final Launcher launcher = new Launcher();
 		assertTrue(launcher.getFactory().CompilationUnit().getMap().isEmpty());
 
-		CtClass myNewClass = launcher.getFactory().createClass("my.new.MyClass");
-		assertEquals(SourcePosition.NOPOSITION, myNewClass.getPosition());
+		CtClass myFooClass = launcher.getFactory().createClass("my.foo.MyClass");
+		assertEquals(SourcePosition.NOPOSITION, myFooClass.getPosition());
 
-		CompilationUnit cu = launcher.getFactory().CompilationUnit().getOrCreate(myNewClass);
+		CompilationUnit cu = launcher.getFactory().CompilationUnit().getOrCreate(myFooClass);
 
 		assertNotNull(cu);
-		assertSame(cu, launcher.getFactory().CompilationUnit().getOrCreate(myNewClass));
-		SourcePosition sourcePosition = myNewClass.getPosition();
+		assertSame(cu, launcher.getFactory().CompilationUnit().getOrCreate(myFooClass));
+		SourcePosition sourcePosition = myFooClass.getPosition();
 		assertTrue(sourcePosition instanceof PartialSourcePositionImpl);
 		assertSame(cu, sourcePosition.getCompilationUnit());
 
-		File f = new File(Launcher.OUTPUTDIR, "my/new/MyClass.java");
+		File f = new File(Launcher.OUTPUTDIR, "my/foo/MyClass.java");
 		assertEquals(f.getCanonicalFile(), cu.getFile());
 	}
 

src/test/java/spoon/test/imports/ImportTest.java

@@ -1507,17 +1507,17 @@ private static String removeIndentation(String str) {
 	public void testImportReferenceIsFullyQualifiedAndNoGeneric() {
 		//contract: the reference of CtImport is always fully qualified and contains no actual type arguments
 		Factory f = new FactoryImpl(new DefaultCoreFactory(), new StandardEnvironment());
-		CtTypeReference<?> typeRef = f.Type().createReference("some.package.SomeType");
+		CtTypeReference<?> typeRef = f.Type().createReference("some.foo.SomeType");
 		typeRef.addActualTypeArgument(f.Type().createTypeParameterReference("T"));
-		assertEquals("some.package.SomeType<T>", typeRef.toString());
+		assertEquals("some.foo.SomeType<T>", typeRef.toString());
 		typeRef.setImplicit(true);
 		typeRef.setSimplyQualified(true);
 		assertTrue(typeRef.isImplicit());
 		assertTrue(typeRef.getPackage().isImplicit());
 		CtImport imprt = f.Type().createImport(typeRef);
 		CtTypeReference<?> typeRef2 = (CtTypeReference<?>) imprt.getReference();
 		assertNotSame(typeRef2, typeRef);
-		assertEquals("some.package.SomeType", typeRef2.toString());
+		assertEquals("some.foo.SomeType", typeRef2.toString());
 		assertFalse(typeRef2.isImplicit());
 		assertFalse(typeRef2.getPackage().isImplicit());
 		//origin reference did not changed

src/test/java/spoon/test/parent/ContractOnSettersParametrizedTest.java

@@ -116,10 +116,10 @@ public static Object createCompatibleObject(CtTypeReference<?> parameterType) {
 		// metamodel elements
 		if (parameterType.toString().equals("spoon.reflect.declaration.CtType<?>")) {
 			CtClass fooBar = f.createClass("FooBar");
-			fooBar.delete();// removing from default package
-			return fooBar; // createNewClass implictly needs a CtClass
+			fooBar.delete(); // removing from default package
+			return fooBar;   // createNewClass implictly needs a CtClass
 		}
-		for(CtType t : allInstantiableMetamodelInterfaces) {
+		for (CtType t : allInstantiableMetamodelInterfaces) {
 			if (c.isAssignableFrom(t.getActualClass())) {
 				CtElement argument = factory.Core().create(t.getActualClass());
 				// an empty package is merged with the existing one
@@ -195,13 +195,13 @@ public static Object createCompatibleObject(CtTypeReference<?> parameterType) {
 
 		throw new IllegalArgumentException("cannot instantiate "+parameterType);
 	}
-	static int nTotalSetterCalls= 0;
+	static int nTotalSetterCalls = 0;
 
 	@Test
 	public void testContract() throws Throwable {
 		factory.getEnvironment().setModelChangeListener(changeListener);
-		int nSetterCalls= 0;
-		int nAssertsOnParent = 0;
+		int nSetterCalls = 0;
+				int nAssertsOnParent = 0;
 		int nAssertsOnParentInList = 0;
 		// contract: all setters/adders must set the parent (not necessarily the direct parent, can be upper in the parent tree, for instance when injecting blocks
 		Object o = factory.Core().create((Class<? extends CtElement>) toTest.getActualClass());
@@ -218,7 +218,7 @@ public void testContract() throws Throwable {
 				Method actualMethod = setter.getReference().getActualMethod();
 
 				int nBefore = changeListener.nbCallsToOnAction;
-				changeListener.changedElements = new ArrayList();
+				changeListener.changedElements = new ArrayList<>();
 
 				// here we actually call the setter
 				actualMethod.invoke(receiver, new Object[] { argument });
@@ -235,8 +235,8 @@ public void testContract() throws Throwable {
 
 				// directly the element
 				if (argument instanceof CtElement
-				  && setter.getAnnotation(UnsettableProperty.class) == null
-				  && setter.getAnnotation(DerivedProperty.class) == null) {
+					&& setter.getAnnotation(UnsettableProperty.class) == null
+					&& setter.getAnnotation(DerivedProperty.class) == null) {
 					nAssertsOnParent++;
 					assertTrue(setter.getDeclaringType().getQualifiedName() + "#" + setter.getSignature() + " doesn't initializes parent", ((CtElement)argument).hasParent(receiver));
 				}

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

@@ -810,7 +810,8 @@ public void testSimpleTemplate() {
 		Factory factory = spoon.getFactory();
 
 		CtClass<?> testSimpleTpl = factory.Class().create("TestSimpleTpl");
-		new SimpleTemplate("Hello world").apply(testSimpleTpl);
+		//whitespace seems wrong here
+		new SimpleTemplate("HelloWorld").apply(testSimpleTpl);
 
 		Set<CtMethod<?>> listMethods = testSimpleTpl.getMethods();
 		assertEquals(0, testSimpleTpl.getMethodsByName("apply").size());
@@ -997,11 +998,11 @@ public void substituteStringLiteral() {
 		}
 		{
 			//contract: simple name of type reference is substituted in String literal 
-			final CtClass<?> result = (CtClass<?>) new SubstituteLiteralTemplate(factory.Type().createReference("some.ignored.package.TypeName")).apply(factory.createClass());
+			final CtClass<?> result = (CtClass<?>) new SubstituteLiteralTemplate(factory.Type().createReference("some.ignored.foo.TypeName")).apply(factory.createClass());
 			assertEquals("java.lang.String stringField1 = \"TypeName\";", result.getField("stringField1").toString());
 			assertEquals("java.lang.String stringField2 = \"Substring TypeName is substituted too - TypeName\";", result.getField("stringField2").toString());
 			//contract type reference is substituted in invocation as class access
-			assertEquals("java.lang.System.out.println(some.ignored.package.TypeName.class)", result.getMethodsByName("m1").get(0).getBody().getStatement(0).toString());
+			assertEquals("java.lang.System.out.println(some.ignored.foo.TypeName.class)", result.getMethodsByName("m1").get(0).getBody().getStatement(0).toString());
 		}
 		{
 			//contract: number literal is substituted in String literal as number converted to string