diff --git a/.github/workflows/verify.yml b/.github/workflows/verify.yml index 8e679d5db..b48d864ce 100644 --- a/.github/workflows/verify.yml +++ b/.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: diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index 4261d5e45..7f2265470 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/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 diff --git a/server/src/__tests__/config.test.ts b/server/src/__tests__/config.test.ts index 747b2cca1..ad5a34df5 100644 --- a/server/src/__tests__/config.test.ts +++ b/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 = {} diff --git a/server/src/__tests__/linter.test.ts b/server/src/__tests__/linter.test.ts new file mode 100644 index 000000000..67d66d4ba --- /dev/null +++ b/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() + }) +}) diff --git a/server/src/config.ts b/server/src/config.ts index 4215aebff..68e6e55f4 100644 --- a/server/src/config.ts +++ b/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() !== '' diff --git a/server/src/linter.ts b/server/src/linter.ts new file mode 100644 index 000000000..3e68ada4f --- /dev/null +++ b/server/src/linter.ts @@ -0,0 +1,202 @@ +import { spawn } from 'child_process' +import * as LSP from 'vscode-languageserver' + +function formatMessage(comment: ShellcheckComment): string { + return (comment.code ? `SC${comment.code}: ` : '') + comment.message +} + +type LinterOptions = { + executablePath: string | null + cwd?: string +} + +export default class Linter { + private executablePath: string | null + private cwd: string + _canLint: boolean + + constructor(opts: LinterOptions) { + this.executablePath = opts.executablePath + this.cwd = opts.cwd || process.cwd() + this._canLint = !!this.executablePath + } + + public get canLint(): boolean { + return this._canLint + } + + public async lint( + document: LSP.TextDocument, + folders: LSP.WorkspaceFolder[], + ): Promise { + if (!this.executablePath || !this._canLint) return [] + + const result = await this.runShellcheck(this.executablePath, document, folders) + if (!this._canLint) return [] + + const diags: LSP.Diagnostic[] = [] + for (const comment of result.comments) { + const start: LSP.Position = { + line: comment.line - 1, + character: comment.column - 1, + } + const end: LSP.Position = { + line: comment.endLine - 1, + character: comment.endColumn - 1, + } + + diags.push({ + message: formatMessage(comment), + severity: mapSeverity(comment.level), + code: comment.code, + source: 'shellcheck', + range: { start, end }, + }) + } + + return diags + } + + private async runShellcheck( + executablePath: string, + document: LSP.TextDocument, + folders: LSP.WorkspaceFolder[], + ): Promise { + const args = ['--format=json1', '--external-sources', `--source-path=${this.cwd}`] + for (const folder of folders) { + args.push(`--source-path=${folder.name}`) + } + + let out = '' + let err = '' + const proc = new Promise((resolve, reject) => { + const proc = spawn(executablePath, [...args, '-'], { cwd: this.cwd }) + proc.on('error', reject) + proc.on('close', resolve) + proc.stdout.on('data', data => (out += data)) + proc.stderr.on('data', data => (err += data)) + proc.stdin.on('error', () => { + // XXX: Ignore STDIN errors in case the process ends too quickly, before we try to + // write. If we write after the process ends without this, we get an uncatchable EPIPE. + // This is solved in Node >= 15.1 by the "on('spawn', ...)" event, but we need to + // support earlier versions. + }) + proc.stdin.end(document.getText()) + }) + + // XXX: do we care about exit code? 0 means "ok", 1 possibly means "errors", + // but the presence of parseable errors in the output is also sufficient to + // distinguish. + let exit + try { + exit = await proc + } catch (e) { + if ((e as any).code === 'ENOENT') { + // shellcheck path wasn't found, don't try to lint any more: + console.error(`shellcheck not available at path '${this.executablePath}'`) + this._canLint = false + return { comments: [] } + } + throw new Error( + `shellcheck failed with code ${exit}: ${e}\nout:\n${out}\nerr:\n${err}`, + ) + } + + let raw + try { + raw = JSON.parse(out) + } catch (e) { + throw new Error( + `shellcheck: json parse failed with error ${e}\nout:\n${out}\nerr:\n${err}`, + ) + } + assertShellcheckResult(raw) + return raw + } +} + +export type ShellcheckResult = Readonly<{ + comments: ReadonlyArray +}> + +// Constituent structures defined here: +// https://github.com/koalaman/shellcheck/blob/4e703e5c61c6366bfd486d728bc624025e344e68/src/ShellCheck/Interface.hs#L221 +export type ShellcheckComment = Readonly<{ + file: string + line: number // 1-based + endLine: number // 1-based + column: number // 1-based + endColumn: number // 1-based + level: string // See mapShellcheckServerity + code: number + message: string + + // The Fix data type appears to be defined here. We aren't making use of + // it yet but this might help down the road: + // https://github.com/koalaman/shellcheck/blob/4e703e5c61c6366bfd486d728bc624025e344e68/src/ShellCheck/Interface.hs#L271 + // fix: any; +}> + +export function assertShellcheckResult(val: any): asserts val is ShellcheckResult { + if (val !== null && typeof val !== 'object') { + throw new Error(`shellcheck: unexpected json output ${typeof val}`) + } + + if (!Array.isArray(val.comments)) { + throw new Error( + `shellcheck: unexpected json output: expected 'comments' array ${typeof val.comments}`, + ) + } + + for (const idx in val.comments) { + const comment = val.comments[idx] + if (comment !== null && typeof comment != 'object') { + throw new Error( + `shellcheck: expected comment at index ${idx} to be object, found ${typeof comment}`, + ) + } + if (typeof comment.file !== 'string') + throw new Error( + `shellcheck: expected comment file at index ${idx} to be string, found ${typeof comment.file}`, + ) + if (typeof comment.level !== 'string') + throw new Error( + `shellcheck: expected comment level at index ${idx} to be string, found ${typeof comment.level}`, + ) + if (typeof comment.message !== 'string') + throw new Error( + `shellcheck: expected comment message at index ${idx} to be string, found ${typeof comment.level}`, + ) + if (typeof comment.line !== 'number') + throw new Error( + `shellcheck: expected comment line at index ${idx} to be number, found ${typeof comment.line}`, + ) + if (typeof comment.endLine !== 'number') + throw new Error( + `shellcheck: expected comment endLine at index ${idx} to be number, found ${typeof comment.endLine}`, + ) + if (typeof comment.column !== 'number') + throw new Error( + `shellcheck: expected comment column at index ${idx} to be number, found ${typeof comment.column}`, + ) + if (typeof comment.endColumn !== 'number') + throw new Error( + `shellcheck: expected comment endColumn at index ${idx} to be number, found ${typeof comment.endColumn}`, + ) + if (typeof comment.code !== 'number') + throw new Error( + `shellcheck: expected comment code at index ${idx} to be number, found ${typeof comment.code}`, + ) + } +} + +const severityMapping: Record = { + error: LSP.DiagnosticSeverity.Error, + warning: LSP.DiagnosticSeverity.Warning, + info: LSP.DiagnosticSeverity.Information, + style: LSP.DiagnosticSeverity.Hint, +} + +// Severity mappings: +// https://github.com/koalaman/shellcheck/blob/364c33395e2f2d5500307f01989f70241c247d5a/src/ShellCheck/Formatter/Format.hs#L50 +const mapSeverity = (sev: string) => severityMapping[sev] || LSP.DiagnosticSeverity.Error diff --git a/server/src/server.ts b/server/src/server.ts index 05dac3ef5..39114d70e 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -1,5 +1,4 @@ import * as Process from 'child_process' -import * as path from 'path' import * as Path from 'path' import * as TurndownService from 'turndown' import * as LSP from 'vscode-languageserver' @@ -9,6 +8,7 @@ import Analyzer from './analyser' import * as Builtins from './builtins' import * as config from './config' import Executables from './executables' +import Linter from './linter' import { initializeParser } from './parser' import * as ReservedWords from './reservedWords' import { BashCompletionItem, CompletionItemDataType } from './types' @@ -41,15 +41,15 @@ export default class BashServer { return Promise.all([ Executables.fromPath(PATH), Analyzer.fromRoot({ connection, rootPath, parser }), - ]).then(xs => { - const executables = xs[0] - const analyzer = xs[1] - return new BashServer(connection, executables, analyzer) + new Linter({ executablePath: config.getShellcheckPath() }), + ]).then(([executables, analyzer, linter]) => { + return new BashServer(connection, executables, analyzer, linter) }) } private executables: Executables private analyzer: Analyzer + private linter: Linter private documents: LSP.TextDocuments = new LSP.TextDocuments(TextDocument) private connection: LSP.Connection @@ -58,10 +58,12 @@ export default class BashServer { connection: LSP.Connection, executables: Executables, analyzer: Analyzer, + linter: Linter, ) { this.connection = connection this.executables = executables this.analyzer = analyzer + this.linter = linter } /** @@ -72,15 +74,21 @@ export default class BashServer { // The content of a text document has changed. This event is emitted // when the text document first opened or when its content has changed. this.documents.listen(this.connection) - this.documents.onDidChangeContent(change => { + this.documents.onDidChangeContent(async change => { const { uri } = change.document - const diagnostics = this.analyzer.analyze(uri, change.document) + let diagnostics: LSP.Diagnostic[] = [] + + // FIXME: re-lint on workspace folder change + const folders = await connection.workspace.getWorkspaceFolders() + const lintDiagnostics = await this.linter.lint(change.document, folders || []) + diagnostics = diagnostics.concat(lintDiagnostics) + if (config.getHighlightParsingError()) { - connection.sendDiagnostics({ - uri: change.document.uri, - diagnostics, - }) + const analyzeDiagnostics = this.analyzer.analyze(uri, change.document) + diagnostics = diagnostics.concat(analyzeDiagnostics) } + + connection.sendDiagnostics({ uri, diagnostics }) }) // Register all the handlers for the LSP events. @@ -164,7 +172,7 @@ export default class BashServer { const symbolDocumentation = commentAboveSymbol ? `\n\n${commentAboveSymbol}` : '' return symbolUri !== currentUri - ? `${symbolKindToDescription(symbol.kind)} defined in ${path.relative( + ? `${symbolKindToDescription(symbol.kind)} defined in ${Path.relative( currentUri, symbolUri, )}${symbolDocumentation}` diff --git a/testing/fixtures.ts b/testing/fixtures.ts index 09be8e85e..88a4eae30 100644 --- a/testing/fixtures.ts +++ b/testing/fixtures.ts @@ -13,6 +13,8 @@ function getDocument(uri: string) { ) } +type FIXTURE_KEY = keyof typeof FIXTURE_URI + export const FIXTURE_URI = { INSTALL: `file://${path.join(FIXTURE_FOLDER, 'install.sh')}`, ISSUE101: `file://${path.join(FIXTURE_FOLDER, 'issue101.sh')}`, @@ -22,16 +24,14 @@ export const FIXTURE_URI = { SOURCING: `file://${path.join(FIXTURE_FOLDER, 'sourcing.sh')}`, COMMENT_DOC: `file://${path.join(FIXTURE_FOLDER, 'comment-doc-on-hover.sh')}`, OPTIONS: `file://${path.join(FIXTURE_FOLDER, 'options.sh')}`, + SHELLCHECK_SOURCE: `file://${path.join(FIXTURE_FOLDER, 'shellcheck', 'source.sh')}`, } -export const FIXTURE_DOCUMENT = { - INSTALL: getDocument(FIXTURE_URI.INSTALL), - ISSUE101: getDocument(FIXTURE_URI.ISSUE101), - MISSING_NODE: getDocument(FIXTURE_URI.MISSING_NODE), - PARSE_PROBLEMS: getDocument(FIXTURE_URI.PARSE_PROBLEMS), - SOURCING: getDocument(FIXTURE_URI.SOURCING), - COMMENT_DOC: getDocument(FIXTURE_URI.COMMENT_DOC), - OPTIONS: getDocument(FIXTURE_URI.OPTIONS), -} +export const FIXTURE_DOCUMENT: Record = (Object.keys( + FIXTURE_URI, +) as Array).reduce((acc, cur: FIXTURE_KEY) => { + acc[cur] = getDocument(FIXTURE_URI[cur]) + return acc +}, {} as any) export default FIXTURE_DOCUMENT diff --git a/testing/fixtures/shellcheck/source.sh b/testing/fixtures/shellcheck/source.sh new file mode 100644 index 000000000..03e5d3dae --- /dev/null +++ b/testing/fixtures/shellcheck/source.sh @@ -0,0 +1,6 @@ +#!/bin/bash + +# shellcheck source=shellcheck/sourced.sh +source ./sourced.sh + +echo "$foo" diff --git a/testing/fixtures/shellcheck/sourced.sh b/testing/fixtures/shellcheck/sourced.sh new file mode 100644 index 000000000..e3897565a --- /dev/null +++ b/testing/fixtures/shellcheck/sourced.sh @@ -0,0 +1,2 @@ +#!/bin/bash +export foo=1 diff --git a/vscode-client/src/extension.ts b/vscode-client/src/extension.ts index 139ae781b..70f5e2f58 100644 --- a/vscode-client/src/extension.ts +++ b/vscode-client/src/extension.ts @@ -19,8 +19,11 @@ export async function activate(context: ExtensionContext) { .getConfiguration('bashIde') .get('highlightParsingErrors', false) + const shellcheckPath = workspace.getConfiguration('bashIde').get('shellcheckPath', '') + const env: any = { ...process.env, + SHELLCHECK_PATH: shellcheckPath, EXPLAINSHELL_ENDPOINT: explainshellEndpoint, GLOB_PATTERN: globPattern, HIGHLIGHT_PARSING_ERRORS: highlightParsingErrors,