Add signature pretty-printer to lsif-semanticdb conversion process #131

Strum355 · 2021-03-29T14:10:54Z

PR adds support for generating a pretty-printed hover text from the emitted SemanticDB Signature and SymbolInformation types.
This happens at the SemanticDB to LSIF stage and is included in the hover results lists for a range, alongside the doc string if it exists.

TODO (either in this PR or a follow-up):

~~annotation interfaces~~
annotations on SymbolInformation (follow up)
additional properties (strictfp/synchronized etc) (follow up)
~~pretty-print enums properly~~
~~implements vs extends~~
Comment outlined here (follow up)
~~skip abstract in interface + interface methods~~
~~constructors~~
~~this keyword references field named this that doesn't exist~~

olafurpg

This is outstanding work! It will be a big milestone when lsif-java supports proper hover messages.

A few comments, mostly minor. The only blocking comments are

print class name instead of <init>
why are there so many unresolved_type in the snapshots?

lsif-java/src/main/scala/com/sourcegraph/lsif_java/SemanticdbPrinters.scala

lsif-java/src/main/scala/com/sourcegraph/lsif_java/commands/SnapshotLsifCommand.scala

olafurpg · 2021-03-31T13:39:03Z

lsif-java/src/main/scala/com/sourcegraph/lsif_java/commands/SnapshotLsifCommand.scala

+            // we cheese it a bit here, as this is less work than trying to reconstruct
+            // a Signature from the pretty-printed Signature, with accompanying logic
+            // to fallback to display_name in SemanticdbPrinters.scala
+            .setDisplayName(hover).setSymbol(symbol).build()


Can we use Documentation instead?

we (currently) dont emit Documentation in the snapshots, only the Signature with DisplayName as fallback.
This should probably still not use Documentation even if we did, if we decide to go ahead with the original "musing" of formatting documentation in a fashion similar to Lorem ipsum ... facilisis, in convallis ex imperdiet., truncating for brevity

I see, the android.view.View class is indeed missing from the classpath that we're using in the snapshots

❯ javap -cp $(cs fetch -r https://maven.google.com/ 'com.airbnb.android:epoxy:4.3.1' -p) android.view.View Error: class not found: android.view.View

The class is defined in android.jar from the SDK

❯ javap -cp $ANDROID_HOME/platforms/android-30/android.jar android.view.View | head Compiled from "View.java" public class android.view.View implements android.graphics.drawable.Drawable$Callback,android.view.KeyEvent$Callback,android.view.accessibility.AccessibilityEventSource { public static final int ACCESSIBILITY_LIVE_REGION_ASSERTIVE; public static final int ACCESSIBILITY_LIVE_REGION_NONE; public static final int ACCESSIBILITY_LIVE_REGION_POLITE; public static final android.util.Property<android.view.View, java.lang.Float> ALPHA; public static final int AUTOFILL_FLAG_INCLUDE_NOT_IMPORTANT_VIEWS; public static final java.lang.String AUTOFILL_HINT_CREDIT_CARD_EXPIRATION_DATE; public static final java.lang.String AUTOFILL_HINT_CREDIT_CARD_EXPIRATION_DAY;

Its weird because many other android types do resolve. 🤷

lsif-semanticdb/src/main/java/com/sourcegraph/lsif_semanticdb/SignatureFormatter.java

tests/snapshots/src/main/generated/com/airbnb/epoxy/AsyncEpoxyController.java

olafurpg · 2021-03-31T13:50:28Z

tests/snapshots/src/main/generated/com/airbnb/epoxy/DiffResult.java

 //                        ^^^^^^^^ reference androidx/annotation/Nullable#
 //                                 ^^^^ reference java/util/List#
 //                                                ^^^^^^^^^^ reference com/airbnb/epoxy/EpoxyModel#
-//                                                               ^^^^^^ definition local0
+//                                                               ^^^^^^ definition local0 List<? extends EpoxyModel<?>> models


olafurpg · 2021-03-31T13:51:16Z

tests/snapshots/src/main/generated/com/airbnb/epoxy/EpoxyAsyncUtil.java

  }

  /**
   * A Handler class that uses the main thread's Looper.
   */
  public static final Handler MAIN_THREAD_HANDLER =
 //                    ^^^^^^^ reference _root_/
-//                            ^^^^^^^^^^^^^^^^^^^ definition com/airbnb/epoxy/EpoxyAsyncUtil#MAIN_THREAD_HANDLER.
+//                            ^^^^^^^^^^^^^^^^^^^ definition com/airbnb/epoxy/EpoxyAsyncUtil#MAIN_THREAD_HANDLER. public static final unresolved_type MAIN_THREAD_HANDLER


Why do we have unresolved_type here?

A lot of android types dont seem to resolve for some reason. You can see that above where the field type is _root_/ instead of the android/os/Handler# that it should be

Strum355 · 2021-03-31T19:57:30Z

why are there so many unresolved_type in the snapshots?

There seems to be some issue with types from the android sdk occasionally (and Im not sure why only occasionally) where they dont resolved. This is always accompanied with the reference occurrence being given the symbol _root_/. Id be curious to see if this happens in non-android-based codebases too.

olafurpg

LGTM 👍 Feel free to merge once you think this is ready to go.

olafurpg · 2021-04-01T08:28:25Z

lsif-java/src/main/scala/com/sourcegraph/lsif_java/commands/SnapshotLsifCommand.scala

+            // we cheese it a bit here, as this is less work than trying to reconstruct
+            // a Signature from the pretty-printed Signature, with accompanying logic
+            // to fallback to display_name in SemanticdbPrinters.scala
+            .setDisplayName(hover).setSymbol(symbol).build()


I see, the android.view.View class is indeed missing from the classpath that we're using in the snapshots

❯ javap -cp $(cs fetch -r https://maven.google.com/ 'com.airbnb.android:epoxy:4.3.1' -p) android.view.View Error: class not found: android.view.View

The class is defined in android.jar from the SDK

❯ javap -cp $ANDROID_HOME/platforms/android-30/android.jar android.view.View | head Compiled from "View.java" public class android.view.View implements android.graphics.drawable.Drawable$Callback,android.view.KeyEvent$Callback,android.view.accessibility.AccessibilityEventSource { public static final int ACCESSIBILITY_LIVE_REGION_ASSERTIVE; public static final int ACCESSIBILITY_LIVE_REGION_NONE; public static final int ACCESSIBILITY_LIVE_REGION_POLITE; public static final android.util.Property<android.view.View, java.lang.Float> ALPHA; public static final int AUTOFILL_FLAG_INCLUDE_NOT_IMPORTANT_VIEWS; public static final java.lang.String AUTOFILL_HINT_CREDIT_CARD_EXPIRATION_DATE; public static final java.lang.String AUTOFILL_HINT_CREDIT_CARD_EXPIRATION_DAY;

olafurpg · 2021-04-01T08:51:22Z

lsif-semanticdb/src/main/java/com/sourcegraph/lsif_semanticdb/LsifSemanticdb.java

        String documentation = symbolInformation.getDocumentation().getMessage();
        if (!documentation.isEmpty()) {
-          int hoverId = writer.emitHoverResult(doc.semanticdb.getLanguage(), documentation);
+          markedStrings.add(new MarkedString(Semanticdb.Language.UNKNOWN_LANGUAGE, documentation));


What do you think about adding a new Documentation.Format called SIGNATURE? We can then make the Documentation documentation = 20; field repeated (which is a binary backwards compatible change). Do you think that would make this cleaner?

Strum355 added 8 commits March 29, 2021 14:46

add SemanticDB symbol parser to lsif-semanticdb

9b8cbe0

change LsifWriter to take varargs of MarkedString for multiple hovers

54402af

add signature pretty-printer skeleton and usage sites

23322e7

add basic class signature formatting

2dbe8af

add basic method signature formatting

d7c07a7

add basic field/param/variable signature formatting

b4f1182

add basic generic type variable signature formatting

7ebdfe5

add modifiers and visibility keywords to signature formatting

2135c05

Strum355 added team/code-intelligence planned/Code Intelligence Iteration 8 labels Mar 29, 2021

Strum355 added this to the Code Intelligence Iteration 9 milestone Mar 29, 2021

Strum355 self-assigned this Mar 29, 2021

This was referenced Mar 29, 2021

Code Intelligence Iteration 8 Tracking issue sourcegraph/sourcegraph-public-snapshot#19137

Closed

Code Intelligence Iteration 9 Tracking issue sourcegraph/sourcegraph-public-snapshot#19533

Closed

Strum355 force-pushed the nsc/lsif-semanticdb-sigformat branch from 0401ab9 to ed1e6ed Compare March 29, 2021 16:30

update snapshots with signature formatter

f170c72

Strum355 force-pushed the nsc/lsif-semanticdb-sigformat branch from ed1e6ed to f170c72 Compare March 29, 2021 16:34

Strum355 added 2 commits March 29, 2021 22:32

fix pretty printing of Array signatures

b284432

fix lsif snapshot diff with extra cheese

4fc0ce9

Strum355 force-pushed the nsc/lsif-semanticdb-sigformat branch from c7586de to 4fc0ce9 Compare March 30, 2021 14:41

improved enum and enum constant pretty printing

2a08071

Strum355 force-pushed the nsc/lsif-semanticdb-sigformat branch from 4a2808f to 2a08071 Compare March 30, 2021 20:19

Strum355 added 4 commits March 30, 2021 21:37

fix 'this' pointing to non-existant field called 'this'

580aabd

fix annotation signature pretty printing

d82dd45

fix adding void|scala/Unit# type to signatures

8bb41e6

fix constructor pretty printing; <init> -> Name

87068e0

olafurpg requested changes Mar 31, 2021

View reviewed changes

Strum355 force-pushed the nsc/lsif-semanticdb-sigformat branch from 38188cb to 6ad078d Compare March 31, 2021 19:54

Strum355 force-pushed the nsc/lsif-semanticdb-sigformat branch from 6ad078d to 7da5641 Compare March 31, 2021 20:29

address review comments

a285b29

Strum355 force-pushed the nsc/lsif-semanticdb-sigformat branch from 7da5641 to a285b29 Compare March 31, 2021 21:11

olafurpg approved these changes Apr 1, 2021

View reviewed changes

add implements vs extends differentiating in pretty-printing

2c6a47d

Strum355 changed the title ~~WIP: add signature pretty-printer to lsif-semanticdb conversion process~~ Add signature pretty-printer to lsif-semanticdb conversion process Apr 1, 2021

Strum355 marked this pull request as ready for review April 1, 2021 21:35

Strum355 merged commit bc5151a into main Apr 1, 2021

Strum355 deleted the nsc/lsif-semanticdb-sigformat branch April 1, 2021 21:37

olafurpg mentioned this pull request Apr 7, 2021

Render Scala-specific signatures in hover messages #142

Closed

Strum355 mentioned this pull request Apr 8, 2021

Add new Documentation.Format kind for Signatures, make repeated and upstream #145

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add signature pretty-printer to lsif-semanticdb conversion process #131

Add signature pretty-printer to lsif-semanticdb conversion process #131

Strum355 commented Mar 29, 2021 •

edited

Loading

olafurpg left a comment

olafurpg Mar 31, 2021

Strum355 Mar 31, 2021

olafurpg Apr 1, 2021

Strum355 Apr 1, 2021

olafurpg Mar 31, 2021

olafurpg Mar 31, 2021

Strum355 Mar 31, 2021

Strum355 commented Mar 31, 2021

olafurpg left a comment

olafurpg Apr 1, 2021

olafurpg Apr 1, 2021

Add signature pretty-printer to lsif-semanticdb conversion process #131

Add signature pretty-printer to lsif-semanticdb conversion process #131

Conversation

Strum355 commented Mar 29, 2021 • edited Loading

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Strum355 commented Mar 31, 2021

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Strum355 commented Mar 29, 2021 •

edited

Loading