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.

Sign up for GitHub

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

Jump to bottom

Added null check to handle anonymous classes to TypeFactory #3553

Closed
wants to merge 1 commit into from
Closed

Added null check to handle anonymous classes to TypeFactory #3553

wants to merge 1 commit into from

Conversation

Strum355
Copy link
Contributor

When indexing https://github.com/google/gson with https://github.com/sourcegraph/lsif-java, anonymous class declarations (of the form com.google.gson.<xyz>.SomeClass$1), the following line would cause a NullPointerException https://github.com/INRIA/spoon/blob/master/src/main/java/spoon/reflect/factory/TypeFactory.java#L568

Was unsuccessful in reproducing at a smaller scale than google/gson. This PR may be just a symptom patch for a larger issue that Im not aware of where to find.

Debugger state view

image

Stack trace 
Exception in thread "main" java.lang.NullPointerException
        at spoon.reflect.factory.TypeFactory.get(TypeFactory.java:571)
        at spoon.support.reflect.reference.CtTypeReferenceImpl.getTypeDeclaration(CtTypeReferenceImpl.java:203)
        at spoon.reflect.visitor.filter.SuperInheritanceHierarchyFunction.sendResult(SuperInheritanceHierarchyFunction.java:353)
        at spoon.reflect.visitor.filter.SuperInheritanceHierarchyFunction.apply(SuperInheritanceHierarchyFunction.java:238)
        at spoon.reflect.visitor.filter.SuperInheritanceHierarchyFunction.apply(SuperInheritanceHierarchyFunction.java:38)
        at spoon.reflect.visitor.chain.CtQueryImpl$LazyFunctionWrapper._accept(CtQueryImpl.java:494)
        at spoon.reflect.visitor.chain.CtQueryImpl$AbstractStep.accept(CtQueryImpl.java:309)
        at spoon.reflect.visitor.chain.CtQueryImpl.forEach(CtQueryImpl.java:95)
        at spoon.reflect.visitor.filter.AllTypeMembersFunction.apply(AllTypeMembersFunction.java:81)
        at spoon.reflect.visitor.filter.AllTypeMembersFunction.apply(AllTypeMembersFunction.java:30)
        at spoon.reflect.visitor.chain.CtQueryImpl$LazyFunctionWrapper._accept(CtQueryImpl.java:494)
        at spoon.reflect.visitor.chain.CtQueryImpl$AbstractStep.accept(CtQueryImpl.java:309)
        at spoon.reflect.visitor.chain.CtQueryImpl.forEach(CtQueryImpl.java:95)
        at spoon.reflect.visitor.filter.PotentialVariableDeclarationFunction.apply(PotentialVariableDeclarationFunction.java:100)
        at spoon.reflect.visitor.filter.PotentialVariableDeclarationFunction.apply(PotentialVariableDeclarationFunction.java:59)
        at spoon.reflect.visitor.chain.CtQueryImpl$LazyFunctionWrapper._accept(CtQueryImpl.java:494)
        at spoon.reflect.visitor.chain.CtQueryImpl$AbstractStep.accept(CtQueryImpl.java:309)
        at spoon.reflect.visitor.chain.CtQueryImpl.first(CtQueryImpl.java:138)
        at spoon.reflect.visitor.chain.CtQueryImpl.first(CtQueryImpl.java:121)
        at spoon.support.reflect.reference.CtLocalVariableReferenceImpl.getDeclaration(CtLocalVariableReferenceImpl.java:62)
        at spoon.support.reflect.reference.CtLocalVariableReferenceImpl.getDeclaration(CtLocalVariableReferenceImpl.java:21)
        at DocumentIndexer$ReferencesVisitor.visitCtVariableRead(DocumentIndexer.java:184)
        at spoon.support.reflect.code.CtVariableReadImpl.accept(CtVariableReadImpl.java:18)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:177)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:169)
        at spoon.reflect.visitor.CtScanner.visitCtInvocation(CtScanner.java:502)
        at DocumentIndexer$ReferencesVisitor.visitCtInvocation(DocumentIndexer.java:199)
        at spoon.support.reflect.code.CtInvocationImpl.accept(CtInvocationImpl.java:46)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:177)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:169)
        at spoon.reflect.visitor.CtScanner.visitCtReturn(CtScanner.java:675)
        at spoon.support.reflect.code.CtReturnImpl.accept(CtReturnImpl.java:27)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:177)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:169)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:142)
        at spoon.reflect.visitor.CtScanner.visitCtBlock(CtScanner.java:299)
        at spoon.support.reflect.code.CtBlockImpl.accept(CtBlockImpl.java:58)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:177)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:169)
        at spoon.reflect.visitor.CtScanner.visitCtMethod(CtScanner.java:556)
        at spoon.support.reflect.declaration.CtMethodImpl.accept(CtMethodImpl.java:58)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:177)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:169)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:142)
        at spoon.reflect.visitor.CtScanner.visitCtClass(CtScanner.java:335)
        at spoon.support.reflect.declaration.CtClassImpl.accept(CtClassImpl.java:58)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:177)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:169)
        at spoon.reflect.visitor.CtScanner.visitCtNewClass(CtScanner.java:601)
        at spoon.support.reflect.code.CtNewClassImpl.accept(CtNewClassImpl.java:25)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:177)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:169)
        at spoon.reflect.visitor.CtScanner.visitCtReturn(CtScanner.java:675)
        at spoon.support.reflect.code.CtReturnImpl.accept(CtReturnImpl.java:27)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:177)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:169)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:142)
        at spoon.reflect.visitor.CtScanner.visitCtBlock(CtScanner.java:299)
        at spoon.support.reflect.code.CtBlockImpl.accept(CtBlockImpl.java:58)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:177)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:169)
        at spoon.reflect.visitor.CtScanner.visitCtTry(CtScanner.java:728)
        at spoon.support.reflect.code.CtTryImpl.accept(CtTryImpl.java:43)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:177)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:169)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:142)
        at spoon.reflect.visitor.CtScanner.visitCtBlock(CtScanner.java:299)
        at spoon.support.reflect.code.CtBlockImpl.accept(CtBlockImpl.java:58)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:177)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:169)
        at spoon.reflect.visitor.CtScanner.visitCtMethod(CtScanner.java:556)
        at spoon.support.reflect.declaration.CtMethodImpl.accept(CtMethodImpl.java:58)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:177)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:169)
        at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:142)
        at spoon.reflect.visitor.CtScanner.visitCtClass(CtScanner.java:335)
        at spoon.support.reflect.declaration.CtClassImpl.accept(CtClassImpl.java:58)
        at DocumentIndexer.visitReferences(DocumentIndexer.java:56)
        at ProjectIndexer.index(ProjectIndexer.java:73)
        at Main.main(Main.java:16)

@rvantonder
Copy link
Contributor

Just some extra context for Spoon folks (CC @monperrus @tdurieux): we're prioritizing better Spoon-powered Java code intelligence at Sourcegraph and @Strum355 is working through our issues in lsif-java, so you'll probably see a couple of PRs like this one to Spoon :-) We're submitting best-effort patches, do let us know if they can be improved. Appreciate your reviews!

@monperrus
Copy link
Collaborator

Thanks a lot for the fix and the context explanation.

How is that possible that newShadowClass is null just after the constructor call new JavaReflectionTreeBuilder?

@Strum355
Copy link
Contributor Author

I will get further context on the state inside scan later today

@Strum355
Copy link
Contributor Author

Additional step-by-step debugger states

JavaReflectionTreeBuilder.scan:L91

image

JavaReflectionTreeBuilder.scan:L94

image

JavaReflectionTreeBuilder.scan:L100

image

EarlyTerminatingScanner.onElement:L173

image

CtTypeImpl.visitCtClass:L371

image

Of note is in CtTypeImpl, setResult is never called, resulting in null being returned.

@monperrus
Copy link
Collaborator

hard to understand with the screenshot.

just for debugging, you can create a PR with a failing test case that uses the full lsif-java stack and gson. We may not merge the test case in master but we will for sure use it to debug and fix the problem.

@Strum355 Strum355 mentioned this pull request Sep 2, 2020
Closed
@monperrus
Copy link
Collaborator

If there is no way to understand this more deeply, while this is indeed a correct fix, we can proceed.

If yes, I would propose to add a fat comment explaining the problem and this workaround and we can merge.

@Strum355
Copy link
Contributor Author

Strum355 commented Sep 2, 2020

I will try get a working test case PR open today/tomorrow if possible. If I cannot get it to reproduce in a test case, we can proceed with your suggestion

@Strum355 Strum355 mentioned this pull request Sep 2, 2020
Closed
@monperrus
Copy link
Collaborator

monperrus commented Sep 5, 2020 •
edited
Loading

Looking at the bug, thanks the failure reproduction test case works perfectly.

It happens for anonymous classes, because Java's Class.getSimpleName "Returns an empty string if the underlying class is anonymous."

Just pushed a tentative fix to see in #3563 whether CI is happy.

@monperrus monperrus mentioned this pull request Sep 5, 2020
Merged
@monperrus
Copy link
Collaborator

#3567 is a proper fix with a minimal test case.

@Strum355 Strum355 mentioned this pull request Sep 5, 2020
Closed
41 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Strum355 @rvantonder @monperrus