Merge pull request #747 from bash-lsp/parse-shellcheck-directives

Use ShellCheck directives when sourcing files

Author: skovhus

server/CHANGELOG.md

@@ -1,5 +1,9 @@
 # Bash Language Server
 
+## 4.8.0
+
+- Use ShellCheck directives when analyzing source commands https://github.com/bash-lsp/bash-language-server/pull/747
+
 ## 4.7.0
 
 - Support for bash options auto completions when using Brew or when `pkg-config` fails, but bash completions are found in `"${PREFIX:-/usr}/share/bash-completion/bash_completion"` https://github.com/bash-lsp/bash-language-server/pull/717

server/package.json

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

server/src/__tests__/analyzer.test.ts

@@ -98,9 +98,10 @@ describe('analyze', () => {
         {
           "message": "Source command could not be analyzed: non-constant source 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.",
+      Consider adding a ShellCheck directive above this line to fix or ignore this:
+      # shellcheck source=/my-file.sh # specify the file to source
+      # shellcheck source-path=my_script_folder # specify the folder to search in
+      # shellcheck source=/dev/null # to ignore the error",
           "range": {
             "end": {
               "character": 16,

server/src/analyser.ts

@@ -100,9 +100,10 @@ export default class Analyzer {
               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.',
+                'Consider adding a ShellCheck directive above this line to fix or ignore this:',
+                '# shellcheck source=/my-file.sh # specify the file to source',
+                '# shellcheck source-path=my_script_folder # specify the folder to search in',
+                '# shellcheck source=/dev/null # to ignore the error',
               ].join('\n'),
               LSP.DiagnosticSeverity.Information,
               undefined,

server/src/shellcheck/__tests__/directive.test.ts

@@ -0,0 +1,135 @@
+import { parseShellCheckDirective } from '../directive'
+
+describe('parseShellCheckDirective', () => {
+  it('parses a disable directive', () => {
+    expect(parseShellCheckDirective('# shellcheck disable=SC1000')).toEqual([
+      {
+        type: 'disable',
+        rules: ['SC1000'],
+      },
+    ])
+  })
+
+  it('parses a disable directive with multiple args', () => {
+    expect(parseShellCheckDirective('# shellcheck disable=SC1000,SC1001')).toEqual([
+      {
+        type: 'disable',
+        rules: ['SC1000', 'SC1001'],
+      },
+    ])
+
+    expect(
+      parseShellCheckDirective(
+        '# shellcheck disable=SC1000,SC2000-SC2002,SC1001 # this is a comment',
+      ),
+    ).toEqual([
+      {
+        type: 'disable',
+        rules: ['SC1000', 'SC2000', 'SC2001', 'SC2002', 'SC1001'],
+      },
+    ])
+
+    expect(parseShellCheckDirective('# shellcheck disable=SC1000,SC1001')).toEqual([
+      {
+        type: 'disable',
+        rules: ['SC1000', 'SC1001'],
+      },
+    ])
+
+    expect(parseShellCheckDirective('# shellcheck disable=SC1000,SC1001')).toEqual([
+      {
+        type: 'disable',
+        rules: ['SC1000', 'SC1001'],
+      },
+    ])
+  })
+
+  // SC1000-SC9999
+  it('parses a disable directive with a range', () => {
+    expect(parseShellCheckDirective('# shellcheck disable=SC1000-SC1005')).toEqual([
+      {
+        type: 'disable',
+        rules: ['SC1000', 'SC1001', 'SC1002', 'SC1003', 'SC1004', 'SC1005'],
+      },
+    ])
+  })
+
+  it('parses a disable directive with all', () => {
+    expect(parseShellCheckDirective('# shellcheck disable=all')).toEqual([
+      {
+        type: 'disable',
+        rules: ['all'],
+      },
+    ])
+  })
+
+  it('parses an enable directive', () => {
+    expect(
+      parseShellCheckDirective('# shellcheck enable=require-variable-braces'),
+    ).toEqual([
+      {
+        type: 'enable',
+        rules: ['require-variable-braces'],
+      },
+    ])
+  })
+
+  it('parses source directive', () => {
+    expect(parseShellCheckDirective('# shellcheck source=foo.sh')).toEqual([
+      {
+        type: 'source',
+        path: 'foo.sh',
+      },
+    ])
+
+    expect(parseShellCheckDirective('# shellcheck source=/dev/null # a comment')).toEqual(
+      [
+        {
+          type: 'source',
+          path: '/dev/null',
+        },
+      ],
+    )
+  })
+
+  it('parses source-path directive', () => {
+    expect(parseShellCheckDirective('# shellcheck source-path=src/examples')).toEqual([
+      {
+        type: 'source-path',
+        path: 'src/examples',
+      },
+    ])
+
+    expect(parseShellCheckDirective('# shellcheck source-path=SCRIPTDIR')).toEqual([
+      {
+        type: 'source-path',
+        path: 'SCRIPTDIR',
+      },
+    ])
+  })
+
+  it('supports multiple directives on the same line', () => {
+    expect(
+      parseShellCheckDirective(
+        `# shellcheck cats=dogs disable=SC1234,SC2345 enable="foo" shell=bash`,
+      ),
+    ).toEqual([
+      {
+        type: 'disable',
+        rules: ['SC1234', 'SC2345'],
+      },
+      {
+        type: 'enable',
+        rules: ['"foo"'],
+      },
+      {
+        type: 'shell',
+        shell: 'bash',
+      },
+    ])
+  })
+
+  it('parses a line with no directive', () => {
+    expect(parseShellCheckDirective('# foo bar')).toEqual([])
+  })
+})

server/src/shellcheck/directive.ts

@@ -0,0 +1,93 @@
+const DIRECTIVE_TYPES = ['enable', 'disable', 'source', 'source-path', 'shell'] as const
+type DirectiveType = (typeof DIRECTIVE_TYPES)[number]
+
+type Directive =
+  | {
+      type: 'enable'
+      rules: string[]
+    }
+  | {
+      type: 'disable'
+      rules: string[]
+    }
+  | {
+      type: 'source'
+      path: string
+    }
+  | {
+      type: 'source-path'
+      path: string
+    }
+  | {
+      type: 'shell'
+      shell: string
+    }
+
+const DIRECTIVE_REG_EXP = /^(#\s*shellcheck\s+)([^#]*)/
+
+export function parseShellCheckDirective(line: string): Directive[] {
+  const match = line.match(DIRECTIVE_REG_EXP)
+
+  if (!match) {
+    return []
+  }
+
+  const commands = match[2]
+    .split(' ')
+    .map((command) => command.trim())
+    .filter((command) => command !== '')
+
+  const directives: Directive[] = []
+
+  for (const command of commands) {
+    const [typeKey, directiveValue] = command.split('=')
+    const type = DIRECTIVE_TYPES.includes(typeKey as any)
+      ? (typeKey as DirectiveType)
+      : null
+
+    if (!type) {
+      continue
+    }
+
+    if (type === 'source-path' || type === 'source') {
+      directives.push({
+        type,
+        path: directiveValue,
+      })
+    } else if (type === 'shell') {
+      directives.push({
+        type,
+        shell: directiveValue,
+      })
+      continue
+    } else if (type === 'enable' || type === 'disable') {
+      const rules = []
+
+      for (const arg of directiveValue.split(',')) {
+        const ruleRangeMatch = arg.match(/^SC(\d*)-SC(\d*)$/)
+        if (ruleRangeMatch) {
+          for (
+            let i = parseInt(ruleRangeMatch[1], 10);
+            i <= parseInt(ruleRangeMatch[2], 10);
+            i++
+          ) {
+            rules.push(`SC${i}`)
+          }
+        } else {
+          arg
+            .split(',')
+            .map((arg) => arg.trim())
+            .filter((arg) => arg !== '')
+            .forEach((arg) => rules.push(arg))
+        }
+      }
+
+      directives.push({
+        type,
+        rules,
+      })
+    }
+  }
+
+  return directives
+}

server/src/util/__tests__/__snapshots__/sourcing.test.ts.snap

@@ -1,6 +1,6 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`getSourcedUris returns a set of sourced files 2`] = `
+exports[`getSourcedUris returns a set of sourced files (but ignores some unhandled cases) 2`] = `
 [
   {
     "error": null,

server/src/util/__tests__/sourcing.test.ts

@@ -2,6 +2,7 @@ import * as fs from 'fs'
 import * as os from 'os'
 import * as Parser from 'web-tree-sitter'
 
+import { REPO_ROOT_FOLDER } from '../../../../testing/fixtures'
 import { initializeParser } from '../../parser'
 import { getSourceCommands } from '../sourcing'
 
@@ -13,8 +14,6 @@ beforeAll(async () => {
   parser = await initializeParser()
 })
 
-jest.spyOn(fs, 'existsSync').mockImplementation(() => true)
-
 // mock os.homedir() to return a fixed path
 jest.spyOn(os, 'homedir').mockImplementation(() => '/Users/bash-user')
 
@@ -29,7 +28,9 @@ describe('getSourcedUris', () => {
     expect(sourceCommands).toEqual([])
   })
 
-  it('returns a set of sourced files', () => {
+  it('returns a set of sourced files (but ignores some unhandled cases)', () => {
+    jest.spyOn(fs, 'existsSync').mockImplementation(() => true)
+
     const fileContent = `
       source file-in-path.sh # does not contain a slash (i.e. is maybe somewhere on the path)
 
@@ -73,7 +74,7 @@ describe('getSourcedUris', () => {
       done
 
       # ======================================
-      # example of sourcing through a function
+      # Example of sourcing through a function
       # ======================================
 
       loadlib () {
@@ -149,4 +150,67 @@ describe('getSourcedUris', () => {
 
     expect(sourceCommands).toMatchSnapshot()
   })
+
+  it('returns a set of sourced files and parses ShellCheck directives', () => {
+    jest.restoreAllMocks()
+
+    const fileContent = `
+      . ./scripts/release-client.sh
+
+      source ./testing/fixtures/issue206.sh
+
+      # shellcheck source=/dev/null
+      source ./IM_NOT_THERE.sh
+
+      # shellcheck source-path=testing/fixtures
+      source missing-node.sh # source path by directive
+
+      # shellcheck source=./testing/fixtures/install.sh
+      source "$X" # source by directive
+
+      # shellcheck source=./some-file-that-does-not-exist.sh
+      source "$Y" # not source due to invalid directive
+
+      # shellcheck source-path=SCRIPTDIR # note that this is already the behaviour of bash language server
+      source ./testing/fixtures/issue101.sh
+      `
+
+    const sourceCommands = getSourceCommands({
+      fileUri,
+      rootPath: REPO_ROOT_FOLDER,
+      tree: parser.parse(fileContent),
+    })
+
+    const sourcedUris = new Set(
+      sourceCommands
+        .map((sourceCommand) => sourceCommand.uri)
+        .filter((uri) => uri !== null),
+    )
+
+    expect(sourcedUris).toEqual(
+      new Set([
+        `file://${REPO_ROOT_FOLDER}/scripts/release-client.sh`,
+        `file://${REPO_ROOT_FOLDER}/testing/fixtures/issue206.sh`,
+        `file://${REPO_ROOT_FOLDER}/testing/fixtures/missing-node.sh`,
+        `file://${REPO_ROOT_FOLDER}/testing/fixtures/install.sh`,
+        `file://${REPO_ROOT_FOLDER}/testing/fixtures/issue101.sh`,
+      ]),
+    )
+
+    expect(
+      sourceCommands
+        .filter((command) => command.error)
+        .map(({ error, range }) => ({
+          error,
+          line: range.start.line,
+        })),
+    ).toMatchInlineSnapshot(`
+      [
+        {
+          "error": "failed to resolve path",
+          "line": 15,
+        },
+      ]
+    `)
+  })
 })

server/src/util/discriminate.ts

@@ -0,0 +1,8 @@
+export function discriminate<K extends PropertyKey, V extends string | number | boolean>(
+  discriminantKey: K,
+  discriminantValue: V,
+) {
+  return <T extends Record<K, any>>(
+    obj: T & Record<K, V extends T[K] ? T[K] : V>,
+  ): obj is Extract<T, Record<K, V>> => obj[discriminantKey] === discriminantValue
+}