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

Cope with missing annotation parameter #691

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

Arthurm1
Copy link
Contributor

@Arthurm1 Arthurm1 commented Apr 5, 2024

This is a quick fix for #686

When @SuppressWarnings("unchecked") is used on a class or method, the TreePathScanner will scan yielding all nodes before scanning the class/method so all those nodes can be used when emitting symbol info e.g. the following nodes for the annotation are all present...

@SuppressWarnings(value = "unchecked")
SuppressWarnings
value = "unchecked"
value
"unchecked"

But when the annotation is added to a variable then the scanner yields...

@SuppressWarnings(value = "unchecked")
SuppressWarnings
"unchecked"

then it scans the variable and then the rest of the annotation. I've no idea why it does it in this order. Notice the value node is missing.

This PR simply stops the link to value being added. It's unused anyway since value doesn't appear in the code and therefore can't be linked to. The reason it's being checked is that javac implicitly adds it during the scan.

If the code actually contains value e.g. @SuppressWarnings(value = "unchecked") then the annotation is scanned before the variable and adds the link correctly. So I don't think this fix should be an issue.

Test plan

I've added no tests for this. Should I add the reproducer code in the issue as a new snapshot test?

@keynmol
Copy link
Contributor

keynmol commented Apr 6, 2024

Hey @Arthurm1 the fix looks as what I'd expect, can you please add the reproducer to snapshot tests? I think this place would fit well: https://github.com/sourcegraph/scip-java/tree/main/tests/minimized/src/main/java/minimized

This is the first NPE I've seen in running scip-java across lots of codebases, so a test would be good to prevent regressions.

@Arthurm1 Arthurm1 force-pushed the missing_annotation_parameter branch from 4f62b19 to 71126bc Compare April 7, 2024 18:50
@Arthurm1
Copy link
Contributor Author

Arthurm1 commented Apr 7, 2024

@keynmol I've added the test supplied in the reproducer. I've run the snapshot on Windows - hopefully that won't cause issues in matching in the CI

@keynmol keynmol merged commit ff5891a into sourcegraph:main Apr 8, 2024
12 checks passed
@keynmol
Copy link
Contributor

keynmol commented Apr 8, 2024

Thanks @Arthurm1!

I'll do a patch release so that Metals can pull it in.

@Arthurm1 Arthurm1 deleted the missing_annotation_parameter branch April 8, 2024 11:56
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.

2 participants