Skip to content

Handle missing cases in CollectAnnotatedSymbolsPsiVisitor#2862

Open
jaschdoc wants to merge 1 commit intomainfrom
fix-psi-visitor-crashes
Open

Handle missing cases in CollectAnnotatedSymbolsPsiVisitor#2862
jaschdoc wants to merge 1 commit intomainfrom
fix-psi-visitor-crashes

Conversation

@jaschdoc
Copy link
Copy Markdown
Collaborator

@jaschdoc jaschdoc commented Apr 9, 2026

I studied the structure of PsiAnnotation a bit further and found what is hopefully an exhaustive solution.

Related to #2816

@jaschdoc jaschdoc requested a review from ting-yuan as a code owner April 9, 2026 12:41
@jaschdoc jaschdoc force-pushed the fix-psi-visitor-crashes branch from fb528fe to a2887fa Compare April 9, 2026 12:42
@jaschdoc jaschdoc requested a review from bcorso April 9, 2026 12:43
@jaschdoc jaschdoc force-pushed the fix-psi-visitor-crashes branch 2 times, most recently from c43931d to ee70d77 Compare April 9, 2026 12:50
return
}

null ->
Copy link
Copy Markdown
Collaborator

@bcorso bcorso Apr 9, 2026

Choose a reason for hiding this comment

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

Why not crash for null and PsiType?

Do you have examples what code leads to these cases?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In particular, I'm curious if you've actually hit issues with this or if this is just to preemptively handle these cases in case they happen. Note that I don't think we technically need to handle every case exhaustively since our visitor itself skips a lot cases we don't care about. Especially, after #2863 where you improve the skip logic, I would be curious if we need to handle null and PsiType. If we don't have explicit failure cases for these then I would lean towards just crashing until someone reports a case and we can investigate the specific issue.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This case was observed when returning an annotation as a default value for another annotation, e.g.,

public @interface JavaAnnotationWithDefaults {
    OtherAnnotation otherAnnotationVal() default @OtherAnnotation("def");
}

See annotationValue_java.kt. In this example, the default value for otherAnnotationVal is @OtherAnnotation("def")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

Copy link
Copy Markdown
Collaborator

@bcorso bcorso left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@ting-yuan ting-yuan requested a review from jsjeon April 9, 2026 21:40
ting-yuan
ting-yuan previously approved these changes Apr 9, 2026
@jaschdoc jaschdoc requested a review from troelsbjerre April 10, 2026 10:57
@jaschdoc jaschdoc force-pushed the fix-psi-visitor-crashes branch from ee70d77 to d22a2a4 Compare April 10, 2026 11:47
@jaschdoc jaschdoc requested a review from bcorso April 10, 2026 11:47
@jaschdoc jaschdoc force-pushed the fix-psi-visitor-crashes branch from d22a2a4 to 5fa573b Compare April 10, 2026 11:50
@jaschdoc jaschdoc requested a review from ting-yuan April 10, 2026 11:50
@jaschdoc jaschdoc force-pushed the fix-psi-visitor-crashes branch 2 times, most recently from ec8b8dd to 3035d09 Compare April 10, 2026 13:27
@jaschdoc jaschdoc force-pushed the fix-psi-visitor-crashes branch from 3035d09 to 701d49f Compare April 10, 2026 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants