Merge pull request #342 from shabbyrobe/master

Shellcheck integration

Author: skovhus

.github/workflows/verify.yml

@@ -14,6 +14,9 @@ jobs:
     steps:
       - uses: actions/checkout@v3
 
+      - name: Install shellcheck (used for testing)
+        run: sudo apt-get install -y shellcheck
+
       - name: Use Node.js ${{ matrix.node-version }}
         uses: actions/setup-node@v3
         with:

server/src/__tests__/analyzer.test.ts

@@ -275,7 +275,7 @@ describe('fromRoot', () => {
     expect(connection.window.showWarningMessage).not.toHaveBeenCalled()
 
     // if you add a .sh file to testing/fixtures, update this value
-    const FIXTURE_FILES_MATCHING_GLOB = 12
+    const FIXTURE_FILES_MATCHING_GLOB = 14
 
     // Intro, stats on glob, one file skipped due to shebang, and outro
     const LOG_LINES = FIXTURE_FILES_MATCHING_GLOB + 4

server/src/__tests__/config.test.ts

@@ -1,5 +1,29 @@
 import * as config from '../config'
 
+describe('getShellcheckPath', () => {
+  it('defaults to shellcheck without path', () => {
+    process.env = {}
+    const result = config.getShellcheckPath()
+    expect(result).toEqual('shellcheck')
+  })
+
+  it('default to null in case of an empty string', () => {
+    process.env = {
+      SHELLCHECK_PATH: '',
+    }
+    const result = config.getShellcheckPath()
+    expect(result).toBeNull()
+  })
+
+  it('parses environment variable', () => {
+    process.env = {
+      SHELLCHECK_PATH: '/path/to/shellcheck',
+    }
+    const result = config.getShellcheckPath()
+    expect(result).toEqual('/path/to/shellcheck')
+  })
+})
+
 describe('getExplainshellEndpoint', () => {
   it('default to null', () => {
     process.env = {}

server/src/__tests__/linter.test.ts

@@ -0,0 +1,154 @@
+import * as path from 'path'
+import * as LSP from 'vscode-languageserver'
+
+import { FIXTURE_DOCUMENT, FIXTURE_FOLDER } from '../../../testing/fixtures'
+import Linter, { assertShellcheckResult } from '../linter'
+
+function textToDoc(txt: string) {
+  return LSP.TextDocument.create('foo', 'bar', 0, txt)
+}
+
+describe('linter', () => {
+  it('should set canLint to false if executable empty', () => {
+    expect(new Linter({ executablePath: null }).canLint).toBe(false)
+  })
+
+  it('should set canLint to true if executable not empty', () => {
+    expect(new Linter({ executablePath: null }).canLint).toBe(false)
+  })
+
+  it('should set canLint to false when linting fails', async () => {
+    jest.spyOn(console, 'error').mockImplementation()
+    const executablePath = '77b4d3f6-c87a-11ec-9b62-a3c90f66d29f'
+    const linter = new Linter({
+      executablePath,
+    })
+    expect(await linter.lint(textToDoc(''), [])).toEqual([])
+    expect(linter.canLint).toBe(false)
+    expect(console.error).toBeCalledWith(
+      expect.stringContaining('shellcheck not available at path'),
+    )
+  })
+
+  it('should lint when shellcheck present', async () => {
+    // prettier-ignore
+    const shell = [
+      '#!/bin/bash',
+      'echo $foo',
+    ].join('\n')
+
+    const expected: LSP.Diagnostic[] = [
+      {
+        message: 'SC2154: foo is referenced but not assigned.',
+        severity: 2,
+        code: 2154,
+        source: 'shellcheck',
+        range: { start: { line: 1, character: 5 }, end: { line: 1, character: 9 } },
+      },
+      {
+        message: 'SC2086: Double quote to prevent globbing and word splitting.',
+        severity: 3,
+        code: 2086,
+        source: 'shellcheck',
+        range: { start: { line: 1, character: 5 }, end: { line: 1, character: 9 } },
+      },
+    ]
+
+    const linter = new Linter({ executablePath: 'shellcheck' })
+    const result = await linter.lint(textToDoc(shell), [])
+    expect(result).toEqual(expected)
+  })
+
+  it('should correctly follow sources with correct cwd', async () => {
+    const linter = new Linter({ executablePath: 'shellcheck', cwd: FIXTURE_FOLDER })
+    const result = await linter.lint(FIXTURE_DOCUMENT.SHELLCHECK_SOURCE, [])
+    expect(result).toEqual([])
+  })
+
+  it('should fail to follow sources with incorrect cwd', async () => {
+    const linter = new Linter({
+      executablePath: 'shellcheck',
+      cwd: path.resolve(path.join(FIXTURE_FOLDER, '../')),
+    })
+    // prettier-ignore
+    const expected = [
+      { message: 'SC1091: Not following: shellcheck/sourced.sh: openBinaryFile: does not exist (No such file or directory)', severity: 3, code: 1091, source: 'shellcheck', range: { start: { line: 3, character: 7 }, end: { line: 3, character: 19 } }, },
+      { message: 'SC2154: foo is referenced but not assigned.', severity: 2, code: 2154, source: 'shellcheck', range: { start: { line: 5, character: 6 }, end: { line: 5, character: 10 } }, },
+    ]
+    const result = await linter.lint(FIXTURE_DOCUMENT.SHELLCHECK_SOURCE, [])
+    expect(result).toEqual(expected)
+  })
+
+  it('should follow sources with incorrect cwd if correct path is passed as a workspace path', async () => {
+    const linter = new Linter({
+      executablePath: 'shellcheck',
+      cwd: path.resolve(path.join(FIXTURE_FOLDER, '../')),
+    })
+    const result = await linter.lint(FIXTURE_DOCUMENT.SHELLCHECK_SOURCE, [
+      { uri: `file://${path.resolve(FIXTURE_FOLDER)}`, name: 'fixtures' },
+    ])
+    expect(result).toEqual([])
+  })
+})
+
+describe('shellcheck', () => {
+  it('asserts one valid shellcheck JSON comment', async () => {
+    // prettier-ignore
+    const shellcheckJSON = {
+      comments: [
+        { file: 'testing/fixtures/comment-doc-on-hover.sh', line: 43, endLine: 43, column: 1, endColumn: 7, level: 'warning', code: 2034, message: 'bork bork', fix: null, },
+      ],
+    }
+    assertShellcheckResult(shellcheckJSON)
+  })
+
+  it('asserts two valid shellcheck JSON comment', async () => {
+    // prettier-ignore
+    const shellcheckJSON = {
+      comments: [
+        { file: 'testing/fixtures/comment-doc-on-hover.sh', line: 43, endLine: 43, column: 1, endColumn: 7, level: 'warning', code: 2034, message: 'bork bork', fix: null, },
+        { file: 'testing/fixtures/comment-doc-on-hover.sh', line: 45, endLine: 45, column: 2, endColumn: 8, level: 'warning', code: 2035, message: 'bork bork', fix: null, },
+      ],
+    }
+    assertShellcheckResult(shellcheckJSON)
+  })
+
+  it('fails shellcheck JSON with null comments', async () => {
+    const shellcheckJSON = { comments: null }
+    expect(() => assertShellcheckResult(shellcheckJSON)).toThrow()
+  })
+
+  it('fails shellcheck JSON with string commment', async () => {
+    const shellcheckJSON = { comments: ['foo'] }
+    expect(() => assertShellcheckResult(shellcheckJSON)).toThrow()
+  })
+
+  it('fails shellcheck JSON with invalid commment', async () => {
+    const make = (tweaks = {}) => ({
+      comments: [
+        {
+          file: 'testing/fixtures/comment-doc-on-hover.sh',
+          line: 43,
+          endLine: 43,
+          column: 1,
+          endColumn: 7,
+          level: 'warning',
+          code: 2034,
+          message: 'bork bork',
+          fix: null,
+          ...tweaks,
+        },
+      ],
+    })
+    assertShellcheckResult(make()) // Defaults should work
+
+    expect(() => assertShellcheckResult(make({ file: 9 }))).toThrow()
+    expect(() => assertShellcheckResult(make({ line: '9' }))).toThrow()
+    expect(() => assertShellcheckResult(make({ endLine: '9' }))).toThrow()
+    expect(() => assertShellcheckResult(make({ column: '9' }))).toThrow()
+    expect(() => assertShellcheckResult(make({ endColumn: '9' }))).toThrow()
+    expect(() => assertShellcheckResult(make({ level: 9 }))).toThrow()
+    expect(() => assertShellcheckResult(make({ code: '9' }))).toThrow()
+    expect(() => assertShellcheckResult(make({ message: 9 }))).toThrow()
+  })
+})

server/src/config.ts

@@ -1,5 +1,11 @@
 export const DEFAULT_GLOB_PATTERN = '**/*@(.sh|.inc|.bash|.command)'
 
+export function getShellcheckPath(): string | null {
+  const { SHELLCHECK_PATH } = process.env
+  // If this is an empty string, this should coalesce to null and disable linting via shellcheck:
+  return typeof SHELLCHECK_PATH === 'string' ? SHELLCHECK_PATH || null : 'shellcheck'
+}
+
 export function getExplainshellEndpoint(): string | null {
   const { EXPLAINSHELL_ENDPOINT } = process.env
   return typeof EXPLAINSHELL_ENDPOINT === 'string' && EXPLAINSHELL_ENDPOINT.trim() !== ''