sourcegraph / scip-java

SCIP Code Intelligence Protocol generator for Java
https://sourcegraph.github.io/scip-java/
Apache License 2.0
66 stars 26 forks source link

Cope with missing annotation parameter #691

Closed Arthurm1 closed 2 months ago

Arthurm1 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

Thanks @Arthurm1!

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