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

Strum355 · 2020-08-25T20:56:59Z

Exception when indexing https://github.com/google/gson library in module mode (with module-info.java and package-info.javas) with https://github.com/sourcegraph/lsif-java with #3549 applied.

Exception does not occur when the library is not a module (with module-info.java and package-info.javas removed) (Note, need to double check the behaviour in each scenario: gson-module+3549, gson-not-module+3549, gson-module, gson-not-module)

Occurs with TypeToken<List<String>[]> @ https://github.com/google/gson/blob/master/gson/src/test/java/com/google/gson/GenericArrayTypeTest.java#L45

Debug state screenshots for context:

Stacktrace

Exception in thread "main" java.lang.StringIndexOutOfBoundsException: String index out of range: 0
        at java.base/java.lang.StringLatin1.charAt(StringLatin1.java:48)
        at java.base/java.lang.String.charAt(String.java:711)
        at spoon.support.reflect.reference.CtReferenceImpl.checkIdentifierChars(CtReferenceImpl.java:129)
        at spoon.support.reflect.reference.CtReferenceImpl.checkAllParts(CtReferenceImpl.java:119)
        at spoon.support.reflect.reference.CtReferenceImpl.checkIdentiferForJLSCorrectness(CtReferenceImpl.java:107)
        at spoon.support.reflect.reference.CtReferenceImpl.setSimpleName(CtReferenceImpl.java:52)
        at spoon.support.compiler.jdt.ReferenceBuilder.getTypeParameterReference(ReferenceBuilder.java:738)
        at spoon.support.compiler.jdt.ReferenceBuilder.getTypeReference(ReferenceBuilder.java:691)
        at spoon.support.compiler.jdt.ReferenceBuilder.getTypeReference(ReferenceBuilder.java:637)
        at spoon.support.compiler.jdt.ReferenceBuilder.getTypeReference(ReferenceBuilder.java:519)
        at spoon.support.compiler.jdt.ReferenceBuilder.getExecutableReference(ReferenceBuilder.java:457)
        at spoon.support.compiler.jdt.JDTTreeBuilder.visit(JDTTreeBuilder.java:907)
        at org.eclipse.jdt.internal.compiler.ast.QualifiedAllocationExpression.traverse(QualifiedAllocationExpression.java:673)
        at org.eclipse.jdt.internal.compiler.ast.MessageSend.traverse(MessageSend.java:1110)
        at org.eclipse.jdt.internal.compiler.ast.LocalDeclaration.traverse(LocalDeclaration.java:480)
        at org.eclipse.jdt.internal.compiler.ast.MethodDeclaration.traverse(MethodDeclaration.java:365)
        at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.traverse(TypeDeclaration.java:1479)
        at org.eclipse.jdt.internal.compiler.ast.CompilationUnitDeclaration.traverse(CompilationUnitDeclaration.java:826)
        at org.eclipse.jdt.internal.compiler.ast.CompilationUnitDeclaration.traverse(CompilationUnitDeclaration.java:787)
        at spoon.support.compiler.jdt.JDTBasedSpoonCompiler.lambda$buildModel$0(JDTBasedSpoonCompiler.java:437)
        at spoon.support.compiler.jdt.JDTBasedSpoonCompiler.forEachCompilationUnit(JDTBasedSpoonCompiler.java:466)
        at spoon.support.compiler.jdt.JDTBasedSpoonCompiler.buildModel(JDTBasedSpoonCompiler.java:435)
        at spoon.support.compiler.jdt.JDTBasedSpoonCompiler.buildUnitsAndModel(JDTBasedSpoonCompiler.java:372)
        at spoon.support.compiler.jdt.JDTBasedSpoonCompiler.buildSources(JDTBasedSpoonCompiler.java:337)
        at spoon.support.compiler.jdt.JDTBasedSpoonCompiler.build(JDTBasedSpoonCompiler.java:114)
        at spoon.support.compiler.jdt.JDTBasedSpoonCompiler.build(JDTBasedSpoonCompiler.java:97)
        at spoon.Launcher.buildModel(Launcher.java:755)
        at ProjectIndexer.index(ProjectIndexer.java:44)
        at Main.main(Main.java:16)

Screenshots

MartinWitt · 2020-08-25T21:11:41Z

Good catch, haven't thought about this case while writing the code. Would you mind adding a new testcase in CorrectIdentifierTest

Strum355 · 2020-08-25T21:22:13Z

Annoyingly, the issue doesnt seem to occur when gson is not a module, with simplename never being == List<String>[], which gives me the impression that there is a more deep seated issue. I lack the knowledge of the codebase to speculate so I'm wondering if you've any ideas?

Strum355 · 2020-08-26T00:40:00Z

Heres the scenario matrix. Patched = with #3549 applied, zero check = with this PR applied, module = if gson has module-info.java and package-info.java

unpatched, zero check, module: Module should be known | expected, should be a module
unpatched, no check, module: Module should be known | expected, should be a module
patched, zero check, module: PartialSourcePosition only contains a CompilationUnit | probably expected
patched, no check, module: String index out of range: 0 | not expected
unpatched, zero check, not module: PartialSourcePosition only contains a CompilationUnit | probably expected
unpatched, no check, not module: PartialSourcePosition only contains a CompilationUnit | probably expected
patched, zero check, not module: PartialSourcePosition only contains a CompilationUnit | probably expected
patched, no check, not module: PartialSourcePosition only contains a CompilationUnit | probably expected

PartialSourcePosition only contains a CompilationUnit is an exception I havent come across before this PR and must investigate further (potential future PR), so I'll consider that as "progress". What I dont get is why the out of range only occurs when gson is a java9 module 🤔

Strum355 · 2020-08-27T10:55:53Z

Have discovered why I got PartialSourcePosition only contains a CompilationUnit, PR coming for that. Why index out of range happens only in module mode is still a mystery

monperrus · 2020-08-28T05:00:19Z

Thanks a lot for the analysis, this is something we've seen in the past.

OK, let's merge the other ones and come back to this one later.

monperrus · 2020-09-09T08:38:14Z

Thanks @Strum355. Could you add the corresponding test case?

Strum355 · 2020-09-09T11:03:25Z

@monperrus test case added 👍

monperrus · 2020-09-09T14:01:07Z

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

+	@Test
+	public void correctSquareBrackets() {
+		CtLocalVariableReference<Object> localVariableRef = new Launcher().getFactory().createLocalVariableReference();
+		assertDoesNotThrow(() -> localVariableRef.setSimpleName("List<String>[]"));


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

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.

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

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 😞

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

LGTM, OK to merge

monperrus · 2020-09-11T07:21:38Z

Thanks @Strum355

Strum355 changed the title ~~Fixed JSLCorrectness fix when simplenamePart == ""~~ Fixed JSLCorrectness check when simplenamePart == "" Aug 25, 2020

MartinWitt approved these changes Aug 25, 2020

View reviewed changes

Strum355 mentioned this pull request Sep 2, 2020

Unable to generate dump on google/gson sourcegraph/scip-java#8

Closed

MartinWitt mentioned this pull request Sep 5, 2020

fix: intersection types with wildcard "?" are allowed as simplename #3568

Merged

Strum355 marked this pull request as ready for review September 8, 2020 19:26

Strum355 force-pushed the jlscorrect-fix branch from 5d39292 to 85411b0 Compare September 9, 2020 10:58

Fixed JSLCorrectness fix when simplenamePart == ""

e6de5e1

Strum355 force-pushed the jlscorrect-fix branch 2 times, most recently from bc483e2 to 80d2ad6 Compare September 9, 2020 11:00

Added test case for JLSCorrectness with simpleName List<String>[]

fd3313d

Strum355 force-pushed the jlscorrect-fix branch from 80d2ad6 to fd3313d Compare September 9, 2020 11:02

monperrus reviewed Sep 9, 2020

View reviewed changes

monperrus added 2 commits September 11, 2020 05:36

Merge branch 'master' into jlscorrect-fix

6a8b0a2

up

b073f05

monperrus changed the title ~~Fixed JSLCorrectness check when simplenamePart == ""~~ fix: fixed identifier correctness check when simplenamePart == "" Sep 11, 2020

monperrus merged commit a14923c into INRIA:master Sep 11, 2020

monperrus mentioned this pull request Oct 14, 2020

chore: release 8.3.0 #3647

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Strum355 commented Aug 25, 2020 •

edited

Loading

MartinWitt commented Aug 25, 2020

Strum355 commented Aug 25, 2020

Strum355 commented Aug 26, 2020

Strum355 commented Aug 27, 2020

monperrus commented Aug 28, 2020

monperrus commented Sep 9, 2020

Strum355 commented Sep 9, 2020

monperrus Sep 9, 2020

MartinWitt Sep 9, 2020 •

edited

Loading

monperrus Sep 10, 2020

Strum355 Sep 10, 2020

monperrus Sep 11, 2020

monperrus commented Sep 11, 2020

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

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

Conversation

Strum355 commented Aug 25, 2020 • edited Loading

MartinWitt commented Aug 25, 2020

Strum355 commented Aug 25, 2020

Strum355 commented Aug 26, 2020

Strum355 commented Aug 27, 2020

monperrus commented Aug 28, 2020

monperrus commented Sep 9, 2020

Strum355 commented Sep 9, 2020

monperrus Sep 9, 2020

Choose a reason for hiding this comment

MartinWitt Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

monperrus Sep 10, 2020

Choose a reason for hiding this comment

Strum355 Sep 10, 2020

Choose a reason for hiding this comment

monperrus Sep 11, 2020

Choose a reason for hiding this comment

monperrus commented Sep 11, 2020

Strum355 commented Aug 25, 2020 •

edited

Loading

MartinWitt Sep 9, 2020 •

edited

Loading