Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fixed identifier correctness check when simplenamePart == "" #3550

Merged
merged 4 commits into from
Sep 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 24 additions & 18 deletions src/main/java/spoon/support/reflect/reference/CtReferenceImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -93,22 +93,22 @@ 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.
/*
* 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);
}
Expand All @@ -122,20 +122,26 @@ private boolean checkAllParts(String[] simplenameParts) {
}
if (isKeyword(simpleName) || checkIdentifierChars(simpleName)) {
return true;
}
}
}
return false;
return false;
}
private boolean checkIdentifierChars(String simplename) {
return (!Character.isJavaIdentifierStart(simplename.charAt(0))) || simplename.chars().anyMatch(letter -> !Character.isJavaIdentifierPart(letter));
if (simplename.length() == 0) {
return false;
}
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));
//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));
}

/**
Expand Down
13 changes: 11 additions & 2 deletions src/test/java/spoon/generating/CorrectIdentifierTest.java
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
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.FluentLauncher;
import spoon.Launcher;
import spoon.SpoonException;
import spoon.reflect.reference.CtLocalVariableReference;
import spoon.reflect.reference.CtTypeReference;

import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertThrows;

/**
* 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.
Expand Down Expand Up @@ -76,4 +79,10 @@ public void intersectionTypeIdentifierTest() {
//contract: intersectionTypes can have simpleNames with '?' for wildcards.
assertDoesNotThrow(() -> new FluentLauncher().inputResource("./src/test/resources/identifier/InliningImplementationMatcher.java").buildModel());
}

@Test
public void correctSquareBrackets() {
CtTypeReference localVariableRef = new Launcher().getFactory().createTypeReference();
assertDoesNotThrow(() -> localVariableRef.setSimpleName("List<String>[]"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that a valid name for a local variable reference? Maybe you mean a type reference instead?

Copy link
Collaborator

@MartinWitt MartinWitt Sep 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't a allowed identifier for a local variable, must be more or less alphanumeric with some exceptions.
We have 1 check for all setSimpleName methods. Looking at the decision today i would prefer something like a visitor. Allowing different handling for the cases., like type or local variable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, @Strum355 could you create a type reference instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My attempts have been unsuccessful unfortunately, I consistently get NPE as getPackage always returns null and Ive been unable to run the Launcher in classpath mode. Im afraid my knowledge of Spoon only goes sofar 😞

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by CtTypeReference localVariableRef = new Launcher().getFactory().createTypeReference();

LGTM, OK to merge

}
}