Skip to content

Commit

Permalink
Fix: multiple compilation units in a file lead to information loss (#738
Browse files Browse the repository at this point in the history 
)

Fixes GRAPH-753

We discovered that javac issues multiple events per file if there are multiple top level definitions that match what javac considers to be a compilation unit - interfaces or classes for example.

These multiple invocations produce slightly different results, and all writing to the same file, meaning the information gets lost.

To fix this, we defensively read the file if it already exists and carefully deduplicate symbols/occurrences/synthetics before writing back.
  • Loading branch information
@antonsviridov-src
antonsviridov-src authored Aug 20, 2024
1 parent 4a6906a commit 2298a95
Show file tree
Hide file tree
Showing 7 changed files with 212 additions and 21 deletions.
1 change: 1 addition & 0 deletions build.sbt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,7 @@ lazy val docs = project
.enablePlugins(DocusaurusPlugin)

lazy val javaOnlySettings = List[Def.Setting[_]](
javafmtOnCompile := false,
autoScalaLibrary := false,
incOptions ~= { old =>
old.withEnabled(false).withApiDebug(true)
Expand Down
115 changes: 105 additions & 10 deletions semanticdb-javac/src/main/java/com/sourcegraph/semanticdb_javac/SemanticdbTaskListener.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,14 @@
import javax.lang.model.util.Types;

import javax.tools.JavaFileObject;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.PrintWriter;
import java.io.*;
import java.net.URI;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.HashSet;
import java.util.Set;
import java.util.stream.Collectors;

/**
* Callback hook that generates SemanticDB when the compiler has completed typechecking a Java
Expand Down Expand Up @@ -44,7 +45,22 @@ public SemanticdbTaskListener(
}

@Override
public void started(TaskEvent e) {}
public void started(TaskEvent e) {
// Upon first encounter with a file (before any other tasks are run)
// we remove the semanticdb file for this source file to ensure
// stale data doesn't cause problems
if (e.getKind() == TaskEvent.Kind.ENTER) {
inferBazelSourceroot(e.getSourceFile());
Result<Path, String> semanticdbPath = semanticdbOutputPath(options, e);
if (semanticdbPath.isOk()) {
try {
Files.deleteIfExists(semanticdbPath.getOrThrow());
} catch (IOException ex) {
this.reportException(ex, e);
}
}
}
}

@Override
public void finished(TaskEvent e) {
Expand Down Expand Up @@ -78,8 +94,10 @@ public void finished(TaskEvent e) {
}
}

// Uses reporter.error with the full stack trace of the exception instead of reporter.exception
// because reporter.exception doesn't seem to print any meaningful information about the
// Uses reporter.error with the full stack trace of the exception instead of
// reporter.exception
// because reporter.exception doesn't seem to print any meaningful information
// about the
// exception, it just prints the location with an empty message.
private void reportException(Throwable exception, TaskEvent e) {
ByteArrayOutputStream baos = new ByteArrayOutputStream();
Expand All @@ -96,7 +114,9 @@ private void onFinishedAnalyze(TaskEvent e) {
Semanticdb.TextDocument textDocument =
new SemanticdbVisitor(globals, e.getCompilationUnit(), options, types, trees, elements)
.buildTextDocument(e.getCompilationUnit());
writeSemanticdb(e, path.getOrThrow(), textDocument);
Path output = path.getOrThrow();
if (Files.exists(output)) appendSemanticdb(e, output, textDocument);
else writeSemanticdb(e, output, textDocument);
} else {
reporter.error(path.getErrorOrThrow(), e);
}
Expand All @@ -114,6 +134,78 @@ private void writeSemanticdb(TaskEvent event, Path output, Semanticdb.TextDocume
}
}

private void appendSemanticdb(
TaskEvent event, Path output, Semanticdb.TextDocument textDocument) {
/*
* If there already is a semanticdb file at the given path,
* we do the following:
* - Read a documents collection
* - Try to find the document with the matching relative path (matching the incoming textDocument)
* - Then, depending on whether a matching document already exists in the collection:
* - if YES, mutate it in place to only add entries from the incoming document
* - if NO, simply add the incoming text document to the collection
* - Write the collection back to disk
* */
Semanticdb.TextDocument document = null;
int documentIndex = -1;
Semanticdb.TextDocuments documents = null;

try (InputStream is = Files.newInputStream(output.toFile().toPath())) {
documents = Semanticdb.TextDocuments.parseFrom(is);

for (int i = 0; i < documents.getDocumentsCount(); i++) {
Semanticdb.TextDocument candidate = documents.getDocuments(i);
if (document == null && candidate.getUri().equals(textDocument.getUri())) {
document = candidate;
documentIndex = i;
}
}

} catch (IOException e) {
this.reportException(e, event);
return;
}

if (document != null) {
// If there is a previous semanticdb document at this path, we need
// to deduplicate symbols and occurrences and mutate the document in place
Set<Semanticdb.SymbolInformation> symbols = new HashSet<>(textDocument.getSymbolsList());
Set<Semanticdb.SymbolOccurrence> occurrences =
new HashSet<>(textDocument.getOccurrencesList());
Set<Semanticdb.Synthetic> synthetics = new HashSet<>(textDocument.getSyntheticsList());

symbols.addAll(document.getSymbolsList());
occurrences.addAll(document.getOccurrencesList());
synthetics.addAll(document.getSyntheticsList());

documents
.toBuilder()
.addDocuments(
documentIndex,
document
.toBuilder()
.clearOccurrences()
.addAllOccurrences(occurrences)
.clearSymbols()
.addAllSymbols(symbols)
.clearSynthetics()
.addAllSynthetics(synthetics));

} else {
// If no prior document was found, we can just add the incoming one to the collection
documents = documents.toBuilder().addDocuments(textDocument).build();
}

byte[] bytes = documents.toByteArray();

try {
Files.createDirectories(output.getParent());
Files.write(output, bytes);
} catch (IOException e) {
this.reportException(e, event);
}
}

public static Path absolutePathFromUri(SemanticdbJavacOptions options, JavaFileObject file) {
URI uri = file.toUri();
if ((options.uriScheme == UriScheme.SBT || options.uriScheme == UriScheme.ZINC)
Expand Down Expand Up @@ -210,10 +302,13 @@ private Result<Path, String> semanticdbOutputPath(SemanticdbJavacOptions options

switch (options.noRelativePath) {
case INDEX_ANYWAY:
// Come up with a unique relative path for this file even if it's not under the sourceroot.
// By indexing auto-generated files, we collect SymbolInformation for auto-generated symbol,
// Come up with a unique relative path for this file even if it's not under the
// sourceroot.
// By indexing auto-generated files, we collect SymbolInformation for
// auto-generated symbol,
// which results in more useful hover tooltips in the editor.
// In the future, we may want to additionally embed the full text contents of these files
// In the future, we may want to additionally embed the full text contents of
// these files
// so that it's possible to browse generated files with precise code navigation.
String uniqueFilename =
String.format("%d.%s.semanticdb", ++noRelativePathCounter, absolutePath.getFileName());
Expand Down
12 changes: 12 additions & 0 deletions tests/minimized/src/main/java/minimized/Interfaces.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,15 @@ default String defaultInterfaceMethod() {
return "default";
}
}

interface BookService {
void checkPages();
}

interface MyService {
BookService bookService();

default void example() {
bookService().checkPages();
}
}
13 changes: 13 additions & 0 deletions ...hots/src/main/generated/tests/minimized/src/main/java/minimized/AnnotationParameters.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
// kind Interface
// relationship is_implementation semanticdb maven jdk 11 java/lang/annotation/Annotation#
→double value();
// ^^^^^ definition semanticdb maven . . minimized/Bar#value().
// display_name value
// signature_documentation java public abstract double value()
// kind AbstractMethod
}

@interface BarB {
Expand All @@ -16,6 +20,10 @@
// kind Interface
// relationship is_implementation semanticdb maven jdk 11 java/lang/annotation/Annotation#
→boolean value();
// ^^^^^ definition semanticdb maven . . minimized/BarB#value().
// display_name value
// signature_documentation java public abstract boolean value()
// kind AbstractMethod
}

@interface Nullable {
Expand All @@ -25,6 +33,11 @@
// kind Interface
// relationship is_implementation semanticdb maven jdk 11 java/lang/annotation/Annotation#
→String value() default "";
//^^^^^ reference semanticdb maven jdk 11 java/lang/String#
// ^^^^^ definition semanticdb maven . . minimized/Nullable#value().
// display_name value
// signature_documentation java public abstract String value()
// kind AbstractMethod
}

interface Foo {
Expand Down
56 changes: 45 additions & 11 deletions ...in/generated/tests/minimized/src/main/java/minimized/AnnotationsOnParameterizedTypes.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,44 @@ public interface AnnotationsOnParameterizedTypes {
// kind Interface

public static AnnotationsOnParameterizedTypes getInstance() {
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ reference semanticdb maven . . minimized/AnnotationsOnParameterizedTypes#
// ^^^^^^^^^^^ definition semanticdb maven . . minimized/AnnotationsOnParameterizedTypes#getInstance().
// display_name getInstance
// signature_documentation java public static AnnotationsOnParameterizedTypes getInstance()
// kind StaticMethod
return new AnnotationsOnParameterizedTypesImpl();
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ reference semanticdb maven . . minimized/AnnotationsOnParameterizedTypesImpl#`<init>`().
}

<C, W> Function<W, C> adapter(Class<C> contract, Class<W> wrappedClass);
// ^ definition semanticdb maven . . minimized/AnnotationsOnParameterizedTypes#adapter().[C]
// display_name C
// signature_documentation java C
// kind TypeParameter
// ^ definition semanticdb maven . . minimized/AnnotationsOnParameterizedTypes#adapter().[W]
// display_name W
// signature_documentation java W
// kind TypeParameter
// ^^^^^^^^ reference semanticdb maven jdk 11 java/util/function/Function#
// ^ reference semanticdb maven . . minimized/AnnotationsOnParameterizedTypes#adapter().[W]
// ^ reference semanticdb maven . . minimized/AnnotationsOnParameterizedTypes#adapter().[C]
// ^^^^^^^ definition semanticdb maven . . minimized/AnnotationsOnParameterizedTypes#adapter().
// display_name adapter
// signature_documentation java public abstract <C, W> Function<W, C> adapter(Class<C> contract, Class<W> wrappedClass)
// kind AbstractMethod
// relationship is_reference is_implementation semanticdb maven . . minimized/AnnotationsOnParameterizedTypesImpl#adapter().
// ^^^^^ reference semanticdb maven jdk 11 java/lang/Class#
// ^ reference semanticdb maven . . minimized/AnnotationsOnParameterizedTypes#adapter().[C]
// ^^^^^^^^ definition local 0
// display_name contract
// signature_documentation java Class<C> contract
// enclosing_symbol semanticdb maven . . minimized/AnnotationsOnParameterizedTypes#adapter().
// ^^^^^ reference semanticdb maven jdk 11 java/lang/Class#
// ^ reference semanticdb maven . . minimized/AnnotationsOnParameterizedTypes#adapter().[W]
// ^^^^^^^^^^^^ definition local 1
// display_name wrappedClass
// signature_documentation java Class<W> wrappedClass
// enclosing_symbol semanticdb maven . . minimized/AnnotationsOnParameterizedTypes#adapter().
}


Expand Down Expand Up @@ -82,13 +116,13 @@ public <C, W> Function<W, C> adapter(Class<C> contract, Class<W> wrappedClass) {
// relationship is_reference is_implementation semanticdb maven . . minimized/AnnotationsOnParameterizedTypes#adapter().
// ^^^^^ reference semanticdb maven jdk 11 java/lang/Class#
// ^ reference semanticdb maven . . minimized/AnnotationsOnParameterizedTypesImpl#adapter().[C]
// ^^^^^^^^ definition local 0
// ^^^^^^^^ definition local 2
// display_name contract
// signature_documentation java Class<C> contract
// enclosing_symbol semanticdb maven . . minimized/AnnotationsOnParameterizedTypesImpl#adapter().
// ^^^^^ reference semanticdb maven jdk 11 java/lang/Class#
// ^ reference semanticdb maven . . minimized/AnnotationsOnParameterizedTypesImpl#adapter().[W]
// ^^^^^^^^^^^^ definition local 1
// ^^^^^^^^^^^^ definition local 3
// display_name wrappedClass
// signature_documentation java Class<W> wrappedClass
// enclosing_symbol semanticdb maven . . minimized/AnnotationsOnParameterizedTypesImpl#adapter().
Expand All @@ -97,19 +131,19 @@ public <C, W> Function<W, C> adapter(Class<C> contract, Class<W> wrappedClass) {
// ^^^^^^^^ reference semanticdb maven jdk 11 java/util/function/Function#
// ^^^^^^^^^^^^^^^^^ reference semanticdb maven jdk 11 java/lang/reflect/InvocationHandler#
// ^ reference semanticdb maven . . minimized/AnnotationsOnParameterizedTypesImpl#adapter().[C]
// ^^^^^^^^^^^ definition local 2
// ^^^^^^^^^^^ definition local 4
// display_name constructor
// signature_documentation java Function<InvocationHandler, C> constructor
// enclosing_symbol semanticdb maven . . minimized/AnnotationsOnParameterizedTypesImpl#adapter().
// kind Variable
// ^^^^^^^^^^^^^^ reference semanticdb maven . . minimized/AnnotationsOnParameterizedTypesImpl#getConstructor().
// ^^^^^^^^ reference local 0
// ^^^^^^^^ reference local 2

System.out.println(constructor);
// ^^^^^^ reference semanticdb maven jdk 11 java/lang/System#
// ^^^ reference semanticdb maven jdk 11 java/lang/System#out.
// ^^^^^^^ reference semanticdb maven jdk 11 java/io/PrintStream#println(+9).
// ^^^^^^^^^^^ reference local 2
// ^^^^^^^^^^^ reference local 4

return null;
}
Expand All @@ -128,7 +162,7 @@ private <T> Function<InvocationHandler, T> getConstructor(Class<T> contract) {
// kind Method
// ^^^^^ reference semanticdb maven jdk 11 java/lang/Class#
// ^ reference semanticdb maven . . minimized/AnnotationsOnParameterizedTypesImpl#getConstructor().[T]
// ^^^^^^^^ definition local 3
// ^^^^^^^^ definition local 5
// display_name contract
// signature_documentation java Class<T> contract
// enclosing_symbol semanticdb maven . . minimized/AnnotationsOnParameterizedTypesImpl#getConstructor().
Expand All @@ -137,17 +171,17 @@ private <T> Function<InvocationHandler, T> getConstructor(Class<T> contract) {
Constructor<T> constructor = (Constructor<T>) proxyConstructors.computeIfAbsent(contract, c -> {
// ^^^^^^^^^^^ reference semanticdb maven jdk 11 java/lang/reflect/Constructor#
// ^ reference semanticdb maven . . minimized/AnnotationsOnParameterizedTypesImpl#getConstructor().[T]
// ^^^^^^^^^^^ definition local 4
// ^^^^^^^^^^^ definition local 6
// display_name constructor
// signature_documentation java @SuppressWarnings("unchecked")\nConstructor<T> constructor
// signature_documentation java @SuppressWarnings\nConstructor<T> constructor
// enclosing_symbol semanticdb maven . . minimized/AnnotationsOnParameterizedTypesImpl#getConstructor().
// kind Variable
// ^^^^^^^^^^^ reference semanticdb maven jdk 11 java/lang/reflect/Constructor#
// ^ reference semanticdb maven . . minimized/AnnotationsOnParameterizedTypesImpl#getConstructor().[T]
// ^^^^^^^^^^^^^^^^^ reference semanticdb maven . . minimized/AnnotationsOnParameterizedTypesImpl#proxyConstructors.
// ^^^^^^^^^^^^^^^ reference semanticdb maven jdk 11 java/util/concurrent/ConcurrentMap#computeIfAbsent().
// ^^^^^^^^ reference local 3
// ^ definition local 5
// ^^^^^^^^ reference local 5
// ^ definition local 7
// display_name c
// signature_documentation java Class<?> c
// enclosing_symbol semanticdb maven . . minimized/AnnotationsOnParameterizedTypesImpl#getConstructor().
Expand All @@ -158,7 +192,7 @@ private <T> Function<InvocationHandler, T> getConstructor(Class<T> contract) {
// ^^^^^^ reference semanticdb maven jdk 11 java/lang/System#
// ^^^ reference semanticdb maven jdk 11 java/lang/System#out.
// ^^^^^^^ reference semanticdb maven jdk 11 java/io/PrintStream#println(+9).
// ^^^^^^^^^^^ reference local 4
// ^^^^^^^^^^^ reference local 6

return null;
}
Expand Down
Loading

0 comments on commit 2298a95

Please sign in to comment.