Merge pull request #673 from bash-lsp/stable-sourcing

Improving sourcing analysis and diagnostics

Author: skovhus

server/CHANGELOG.md

@@ -1,5 +1,14 @@
 # Bash Language Server
 
+## 4.4.0
+
+- Improve source command parser and include diagnostics when parser fails https://github.com/bash-lsp/bash-language-server/pull/673
+- Fix `onHover` bug where sourced symbols on the same line as a reference would hide documentation https://github.com/bash-lsp/bash-language-server/pull/673
+
+## 4.3.2
+
+- Improved CLI output https://github.com/bash-lsp/bash-language-server/pull/672
+
 ## 4.3.0
 
 - Add centralized and configurable logger that can be controlled using the `BASH_IDE_LOG_LEVEL` environment variable and workspace configuration. https://github.com/bash-lsp/bash-language-server/pull/669

server/package.json

@@ -3,7 +3,7 @@
   "description": "A language server for Bash",
   "author": "Mads Hartmann",
   "license": "MIT",
-  "version": "4.3.2",
+  "version": "4.4.0",
   "main": "./out/server.js",
   "typings": "./out/server.d.ts",
   "bin": {

server/src/__tests__/__snapshots__/analyzer.test.ts.snap

@@ -1,43 +1,5 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`analyze returns a list of errors for a file with a missing node 1`] = `
-Array [
-  Object {
-    "message": "Syntax error: expected \\"fi\\" somewhere in the file",
-    "range": Object {
-      "end": Object {
-        "character": 0,
-        "line": 12,
-      },
-      "start": Object {
-        "character": 0,
-        "line": 12,
-      },
-    },
-    "severity": 2,
-  },
-]
-`;
-
-exports[`analyze returns a list of errors for a file with parsing errors 1`] = `
-Array [
-  Object {
-    "message": "Failed to parse",
-    "range": Object {
-      "end": Object {
-        "character": 1,
-        "line": 9,
-      },
-      "start": Object {
-        "character": 0,
-        "line": 2,
-      },
-    },
-    "severity": 1,
-  },
-]
-`;
-
 exports[`findReferences returns a list of locations if parameter is found 1`] = `
 Array [
   Object {

server/src/__tests__/analyzer.test.ts

@@ -36,30 +36,110 @@ beforeAll(async () => {
 })
 
 describe('analyze', () => {
-  it('returns an empty list of errors for a file with no parsing errors', () => {
-    const result = analyzer.analyze({
+  it('returns an empty list of diagnostics for a file with no parsing errors', () => {
+    const diagnostics = analyzer.analyze({
       uri: CURRENT_URI,
       document: FIXTURE_DOCUMENT.INSTALL,
     })
-    expect(result).toEqual([])
+    expect(diagnostics).toEqual([])
   })
 
-  it('returns a list of errors for a file with a missing node', () => {
-    const result = analyzer.analyze({
+  it('returns a list of diagnostics for a file with a missing node', () => {
+    const diagnostics = analyzer.analyze({
       uri: CURRENT_URI,
       document: FIXTURE_DOCUMENT.MISSING_NODE,
     })
-    expect(result).not.toEqual([])
-    expect(result).toMatchSnapshot()
+    expect(diagnostics).not.toEqual([])
+    expect(diagnostics).toMatchInlineSnapshot(`
+      Array [
+        Object {
+          "message": "Syntax error: expected \\"fi\\" somewhere in the file",
+          "range": Object {
+            "end": Object {
+              "character": 0,
+              "line": 12,
+            },
+            "start": Object {
+              "character": 0,
+              "line": 12,
+            },
+          },
+          "severity": 2,
+          "source": "bash-language-server",
+        },
+      ]
+    `)
   })
 
-  it('returns a list of errors for a file with parsing errors', () => {
-    const result = analyzer.analyze({
+  it('returns a list of diagnostics for a file with parsing errors', () => {
+    const diagnostics = analyzer.analyze({
       uri: CURRENT_URI,
       document: FIXTURE_DOCUMENT.PARSE_PROBLEMS,
     })
-    expect(result).not.toEqual([])
-    expect(result).toMatchSnapshot()
+    expect(diagnostics).not.toEqual([])
+    expect(diagnostics).toMatchInlineSnapshot(`
+      Array [
+        Object {
+          "message": "Failed to parse",
+          "range": Object {
+            "end": Object {
+              "character": 1,
+              "line": 9,
+            },
+            "start": Object {
+              "character": 0,
+              "line": 2,
+            },
+          },
+          "severity": 1,
+          "source": "bash-language-server",
+        },
+      ]
+    `)
+  })
+
+  it('returns a list of diagnostics for a file with sourcing issues', async () => {
+    const parser = await initializeParser()
+    const newAnalyzer = new Analyzer({
+      parser,
+      workspaceFolder: FIXTURE_FOLDER,
+    })
+    const diagnostics = newAnalyzer.analyze({
+      uri: CURRENT_URI,
+      document: FIXTURE_DOCUMENT.SOURCING,
+    })
+    expect(diagnostics).not.toEqual([])
+    expect(diagnostics).toMatchInlineSnapshot(`
+      Array [
+        Object {
+          "message": "Source command could not be analyzed: expansion not supported.
+
+      Note that enabling the configuration flag \\"includeAllWorkspaceSymbols\\"
+      would include all symbols in the workspace regardless of source commands.
+      But be aware that this will lead to false positive suggestions.",
+          "range": Object {
+            "end": Object {
+              "character": 16,
+              "line": 21,
+            },
+            "start": Object {
+              "character": 2,
+              "line": 21,
+            },
+          },
+          "severity": 3,
+          "source": "bash-language-server",
+        },
+      ]
+    `)
+
+    // unless setIncludeAllWorkspaceSymbols set
+    newAnalyzer.setIncludeAllWorkspaceSymbols(true)
+    const diagnostics2 = newAnalyzer.analyze({
+      uri: CURRENT_URI,
+      document: FIXTURE_DOCUMENT.SOURCING,
+    })
+    expect(diagnostics2).toEqual([])
   })
 })
 

server/src/__tests__/server.test.ts

@@ -208,6 +208,23 @@ describe('server', () => {
           },
           "name": "BOLD",
         },
+        Object {
+          "kind": 12,
+          "location": Object {
+            "range": Object {
+              "end": Object {
+                "character": 1,
+                "line": 22,
+              },
+              "start": Object {
+                "character": 0,
+                "line": 20,
+              },
+            },
+            "uri": "file://${FIXTURE_FOLDER}sourcing.sh",
+          },
+          "name": "loadlib",
+        },
       ]
     `)
   })

server/src/analyser.ts

@@ -17,6 +17,7 @@ import {
 } from './util/declarations'
 import { getFilePaths } from './util/fs'
 import { logger } from './util/logger'
+import { isPositionIncludedInRange } from './util/lsp'
 import { analyzeShebang } from './util/shebang'
 import * as sourcing from './util/sourcing'
 import * as TreeSitterUtil from './util/tree-sitter'
@@ -27,6 +28,7 @@ type AnalyzedDocument = {
   document: TextDocument
   globalDeclarations: GlobalDeclarations
   sourcedUris: Set<string>
+  sourceCommands: sourcing.SourceCommand[]
   tree: Parser.Tree
 }
 
@@ -74,25 +76,56 @@ export default class Analyzer {
 
     const { diagnostics, globalDeclarations } = getGlobalDeclarations({ tree, uri })
 
+    const sourceCommands = sourcing.getSourceCommands({
+      fileUri: uri,
+      rootPath: this.workspaceFolder,
+      tree,
+    })
+
+    const sourcedUris = new Set(
+      sourceCommands
+        .map((sourceCommand) => sourceCommand.uri)
+        .filter((uri): uri is string => uri !== null),
+    )
+
     this.uriToAnalyzedDocument[uri] = {
       document,
       globalDeclarations,
-      sourcedUris: sourcing.getSourcedUris({
-        fileContent,
-        fileUri: uri,
-        rootPath: this.workspaceFolder,
-        tree,
-      }),
+      sourcedUris,
+      sourceCommands: sourceCommands.filter((sourceCommand) => !sourceCommand.error),
       tree,
     }
 
+    if (!this.includeAllWorkspaceSymbols) {
+      sourceCommands
+        .filter((sourceCommand) => sourceCommand.error)
+        .forEach((sourceCommand) => {
+          diagnostics.push(
+            LSP.Diagnostic.create(
+              sourceCommand.range,
+              [
+                `Source command could not be analyzed: ${sourceCommand.error}.\n`,
+                'Note that enabling the configuration flag "includeAllWorkspaceSymbols"',
+                'would include all symbols in the workspace regardless of source commands.',
+                'But be aware that this will lead to false positive suggestions.',
+              ].join('\n'),
+              LSP.DiagnosticSeverity.Information,
+              undefined,
+              'bash-language-server',
+            ),
+          )
+        })
+    }
+
     function findMissingNodes(node: Parser.SyntaxNode) {
       if (node.isMissing()) {
         diagnostics.push(
           LSP.Diagnostic.create(
             TreeSitterUtil.range(node),
             `Syntax error: expected "${node.type}" somewhere in the file`,
             LSP.DiagnosticSeverity.Warning,
+            undefined,
+            'bash-language-server',
           ),
         )
       } else if (node.hasError()) {
@@ -198,20 +231,13 @@ export default class Analyzer {
     uri: string
     word: string
   }): LSP.Location[] {
-    const tree = this.uriToAnalyzedDocument[uri]?.tree
+    // If the word is sourced, return the location of the source file
+    const sourcedUri = this.uriToAnalyzedDocument[uri]?.sourceCommands
+      .filter((sourceCommand) => isPositionIncludedInRange(position, sourceCommand.range))
+      .map((sourceCommand) => sourceCommand.uri)[0]
 
-    if (position && tree) {
-      // NOTE: when a word is a file path to a sourced file, we return a location to it.
-      const sourcedLocation = sourcing.getSourcedLocation({
-        position,
-        rootPath: this.workspaceFolder,
-        tree,
-        uri,
-        word,
-      })
-      if (sourcedLocation) {
-        return [sourcedLocation]
-      }
+    if (sourcedUri) {
+      return [LSP.Location.create(sourcedUri, LSP.Range.create(0, 0, 0, 0))]
     }
 
     return this.findDeclarationsMatchingWord({

server/src/config.ts

@@ -61,7 +61,8 @@ export function getConfigFromEnvironmentVariables(): {
 
   const environmentVariablesUsed = Object.entries(rawConfig)
     .map(([key, value]) => (typeof value !== 'undefined' ? key : null))
-    .filter((key) => key !== null) as string[]
+    .filter((key): key is string => key !== null)
+    .filter((key) => key !== 'logLevel') // logLevel is a special case that we ignore
 
   const config = ConfigSchema.parse(rawConfig)
 

server/src/server.ts

@@ -364,6 +364,9 @@ export default class BashServer {
     symbol: LSP.SymbolInformation
     currentUri: string
   }): LSP.MarkupContent {
+    logger.debug(
+      `getDocumentationForSymbol: symbol=${symbol.name} uri=${symbol.location.uri}`,
+    )
     const symbolUri = symbol.location.uri
     const symbolStartLine = symbol.location.range.start.line
 
@@ -665,6 +668,9 @@ export default class BashServer {
       Builtins.isBuiltin(word) ||
       (this.executables.isExecutableOnPATH(word) && symbolsMatchingWord.length == 0)
     ) {
+      logger.debug(
+        `onHover: getting shell documentation for reserved word or builtin or executable`,
+      )
       const shellDocumentation = await getShellDocumentation({ word })
       if (shellDocumentation) {
         return { contents: getMarkdownContent(shellDocumentation, 'man') }
@@ -675,7 +681,11 @@ export default class BashServer {
         currentUri,
       })
         // do not return hover referencing for the current line
-        .filter((symbol) => symbol.location.range.start.line !== params.position.line)
+        .filter(
+          (symbol) =>
+            symbol.location.uri !== currentUri ||
+            symbol.location.range.start.line !== params.position.line,
+        )
         .map((symbol: LSP.SymbolInformation) =>
           this.getDocumentationForSymbol({ currentUri, symbol }),
         )