From 4e1c74126887bc3c6860e9b227fb5b3c4e1a846c Mon Sep 17 00:00:00 2001 From: TJ DeVries Date: Wed, 20 Jul 2022 11:26:16 -0400 Subject: [PATCH 1/2] fix: skip exported symbols with no definition --- bindings/go/scip/convert.go | 20 ++++++++++++++++++++ bindings/go/scip/document.go | 11 +++++++++++ 2 files changed, 31 insertions(+) create mode 100644 bindings/go/scip/document.go diff --git a/bindings/go/scip/convert.go b/bindings/go/scip/convert.go index 3aeb1d76..14a51185 100644 --- a/bindings/go/scip/convert.go +++ b/bindings/go/scip/convert.go @@ -65,12 +65,24 @@ func ConvertSCIPToLSIF(index *Index) ([]reader.Element, error) { }, ) + // Emit a warning with the symbols for a document that were exported + // but had no definition. Only display one message so that there are fewer warnings + // displayed to users. + exportedSymbolsWithNoDefinition := []string{} + // Pass 1: create result sets for global symbols. for _, importedSymbol := range index.ExternalSymbols { g.symbolToResultSet[importedSymbol.Symbol] = g.emitResultSet(importedSymbol, "import") } for _, document := range index.Documents { for _, exportedSymbol := range document.Symbols { + // If an exported symbol is not defined in a document, + // do not emit a definition for this symbol. + if !document.HasDefinition(exportedSymbol) { + exportedSymbolsWithNoDefinition = append(exportedSymbolsWithNoDefinition, exportedSymbol.Symbol) + continue + } + g.registerInverseRelationships(exportedSymbol) if IsGlobalSymbol(exportedSymbol.Symbol) { // Local symbols are skipped here because we handle them in the @@ -97,6 +109,14 @@ func ConvertSCIPToLSIF(index *Index) ([]reader.Element, error) { g.emitDocument(index, document) } + // TODO: We could instead pass back a list of warnings to be displayed by callers? + // I don't really like just printing to stdout here (additionally, these could be coallesced between documents) + if len(exportedSymbolsWithNoDefinition) > 0 { + fmt.Println("[scip-go] Warning: The following exported symbols were ignored because they were missing a definition") + fmt.Println(" (did you intend to put this symbol into Index.external_symbols?)") + fmt.Println(exportedSymbolsWithNoDefinition) + } + return g.Elements, nil } diff --git a/bindings/go/scip/document.go b/bindings/go/scip/document.go new file mode 100644 index 00000000..0913fd63 --- /dev/null +++ b/bindings/go/scip/document.go @@ -0,0 +1,11 @@ +package scip + +func (d *Document) HasDefinition(symbol *SymbolInformation) bool { + for _, occ := range d.Occurrences { + if occ.Symbol == symbol.Symbol && SymbolRole_Definition.Matches(occ) { + return true + } + } + + return false +} From 6e53a13bff09eaa49d4a62c6137851037118a197 Mon Sep 17 00:00:00 2001 From: TJ DeVries Date: Wed, 20 Jul 2022 16:24:09 -0400 Subject: [PATCH 2/2] example possibility, can be reverted if we don't like it --- bindings/go/scip/convert.go | 26 ++++++++---------- bindings/go/scip/index_diagnostics.go | 39 +++++++++++++++++++++++++++ cmd/convert.go | 13 +++++++-- cmd/main_test.go | 2 +- 4 files changed, 62 insertions(+), 18 deletions(-) create mode 100644 bindings/go/scip/index_diagnostics.go diff --git a/bindings/go/scip/convert.go b/bindings/go/scip/convert.go index 14a51185..e7965fa7 100644 --- a/bindings/go/scip/convert.go +++ b/bindings/go/scip/convert.go @@ -32,14 +32,14 @@ import ( // such as asymmetric references/definitions. // // This conversion functionality is used by src-cli. -func ConvertSCIPToLSIF(index *Index) ([]reader.Element, error) { +func ConvertSCIPToLSIF(index *Index) ([]reader.Element, map[string]IndexDiagnostic, error) { g := newGraph() if index.Metadata == nil { - return nil, errors.New(".Metadata is nil") + return nil, nil, errors.New(".Metadata is nil") } if index.Metadata.ToolInfo == nil { - return nil, errors.New(".Metadata.ToolInfo is nil") + return nil, nil, errors.New(".Metadata.ToolInfo is nil") } positionEncoding := "" @@ -49,7 +49,7 @@ func ConvertSCIPToLSIF(index *Index) ([]reader.Element, error) { case TextEncoding_UTF16: positionEncoding = "utf-16" default: - return nil, errors.New(".Metadata.TextDocumentEncoding does not have value utf-8 or utf-16") + return nil, nil, errors.New(".Metadata.TextDocumentEncoding does not have value utf-8 or utf-16") } g.emitVertex( @@ -68,7 +68,7 @@ func ConvertSCIPToLSIF(index *Index) ([]reader.Element, error) { // Emit a warning with the symbols for a document that were exported // but had no definition. Only display one message so that there are fewer warnings // displayed to users. - exportedSymbolsWithNoDefinition := []string{} + indexDiagnostics := map[string]IndexDiagnostic{} // Pass 1: create result sets for global symbols. for _, importedSymbol := range index.ExternalSymbols { @@ -79,7 +79,11 @@ func ConvertSCIPToLSIF(index *Index) ([]reader.Element, error) { // If an exported symbol is not defined in a document, // do not emit a definition for this symbol. if !document.HasDefinition(exportedSymbol) { - exportedSymbolsWithNoDefinition = append(exportedSymbolsWithNoDefinition, exportedSymbol.Symbol) + diag := NoSymbolDefinitionDiagnostic{ + symbol: exportedSymbol, + } + indexDiagnostics[diag.Key()] = diag + continue } @@ -109,15 +113,7 @@ func ConvertSCIPToLSIF(index *Index) ([]reader.Element, error) { g.emitDocument(index, document) } - // TODO: We could instead pass back a list of warnings to be displayed by callers? - // I don't really like just printing to stdout here (additionally, these could be coallesced between documents) - if len(exportedSymbolsWithNoDefinition) > 0 { - fmt.Println("[scip-go] Warning: The following exported symbols were ignored because they were missing a definition") - fmt.Println(" (did you intend to put this symbol into Index.external_symbols?)") - fmt.Println(exportedSymbolsWithNoDefinition) - } - - return g.Elements, nil + return g.Elements, indexDiagnostics, nil } // graph is a helper struct to emit LSIF. diff --git a/bindings/go/scip/index_diagnostics.go b/bindings/go/scip/index_diagnostics.go new file mode 100644 index 00000000..feb44d3c --- /dev/null +++ b/bindings/go/scip/index_diagnostics.go @@ -0,0 +1,39 @@ +package scip + +import "fmt" + +type IndexDiagnosticType int + +const ( + ExportedSymbolsWithNoDefinition IndexDiagnosticType = iota +) + +type IndexDiagnostic interface { + // Key returns a unique key for this diagnostic. It allows for easy deduplication with a Map + Key() string + + // DiagnosticType returns the index type. Can be used to switch case over different types + // if we wanted to do anything special with them. + DiagnosticType() IndexDiagnosticType + + // Message to display to user if calling code wants to display any information + Message() string +} + +// NoSymbolDefinitionDiagnostic is used when a Symbol was exported by a document, +// but there was no corresponding definition occurrence in the document. +// +// We discard these symbols in the indexer, but with this we can warn the user as well. +type NoSymbolDefinitionDiagnostic struct { + symbol *SymbolInformation +} + +func (NoSymbolDefinitionDiagnostic) DiagnosticType() IndexDiagnosticType { + return ExportedSymbolsWithNoDefinition +} +func (diag NoSymbolDefinitionDiagnostic) Message() string { + return fmt.Sprintf("No Definition For Exported Symbol: %s", diag.symbol.Symbol) +} +func (diag NoSymbolDefinitionDiagnostic) Key() string { + return fmt.Sprintf("%d:%s", diag.DiagnosticType(), diag.symbol.Symbol) +} diff --git a/cmd/convert.go b/cmd/convert.go index f3007171..014edba7 100644 --- a/cmd/convert.go +++ b/cmd/convert.go @@ -1,6 +1,7 @@ package main import ( + "fmt" "io" "os" "strings" @@ -29,7 +30,7 @@ func convertCommand() cli.Command { Name: "to", Usage: "Output path for LSIF index", Destination: &convertFlags.to, - Value: "dump.lsif", + Value: "dump.lsif", }, }, Action: func(c *cli.Context) error { @@ -60,11 +61,19 @@ func convertMain(flags convertFlags) error { lsifWriter = lsifFile } - lsifIndex, err := scip.ConvertSCIPToLSIF(scipIndex) + lsifIndex, diagnostics, err := scip.ConvertSCIPToLSIF(scipIndex) if err != nil { return errors.Wrap(err, "failed to convert SCIP index to LSIF index") } + if len(diagnostics) > 0 { + fmt.Println("The following warnings occurred while processing SCIP index:") + fmt.Println("====================") + for _, diag := range diagnostics { + fmt.Println(diag.Message()) + } + } + err = reader.WriteNDJSON(reader.ElementsToJsonElements(lsifIndex), lsifWriter) if err != nil { return errors.Wrapf(err, "failed to write LSIF index to path %s", toPath) diff --git a/cmd/main_test.go b/cmd/main_test.go index 4cb06dbf..229a4cfe 100644 --- a/cmd/main_test.go +++ b/cmd/main_test.go @@ -87,7 +87,7 @@ func TestSCIPSnapshots(t *testing.T) { snapshots, err := testutil.FormatSnapshots(index, "#", symbolFormatter) require.Nil(t, err) index.Metadata.ProjectRoot = "file:/root" - lsif, err := scip.ConvertSCIPToLSIF(index) + lsif, _, err := scip.ConvertSCIPToLSIF(index) require.Nil(t, err) var obtained bytes.Buffer err = reader.WriteNDJSON(reader.ElementsToJsonElements(lsif), &obtained)