From 9b4afc9285dd047fcc0b1f3419e1ddd4edd5cb41 Mon Sep 17 00:00:00 2001 From: skovhus Date: Mon, 28 Nov 2022 10:17:26 +0100 Subject: [PATCH 1/3] Remove ShellCheck executable lookup So we have a few different use cases here: 1) being able to disable shellcheck linting 2) being able to provide a custom path for shellcheck 3) default behaviour of the shellcheck integration working if shellcheck is found on the path Defaulting to "shellcheck" and making an empty string disable it solves all of them. --- server/package.json | 3 +-- server/src/linter.ts | 27 ++++++++++++------------ server/src/server.ts | 50 +++++++++++++++++++++++++------------------- server/yarn.lock | 12 ----------- 4 files changed, 42 insertions(+), 50 deletions(-) diff --git a/server/package.json b/server/package.json index 4c71af188..de960867e 100644 --- a/server/package.json +++ b/server/package.json @@ -26,8 +26,7 @@ "urijs": "^1.19.11", "vscode-languageserver": "^6.1.1", "vscode-languageserver-textdocument": "^1.0.1", - "web-tree-sitter": "^0.20.5", - "which": "^2.0.2" + "web-tree-sitter": "^0.20.5" }, "scripts": { "prepublishOnly": "cd ../ && yarn run compile" diff --git a/server/src/linter.ts b/server/src/linter.ts index fc71eb2b6..9734eefc9 100644 --- a/server/src/linter.ts +++ b/server/src/linter.ts @@ -1,8 +1,5 @@ import { spawn } from 'child_process' import * as LSP from 'vscode-languageserver' -import which = require('which') - -import * as config from './config' function formatMessage(comment: ShellcheckComment): string { return (comment.code ? `SC${comment.code}: ` : '') + comment.message @@ -10,22 +7,22 @@ function formatMessage(comment: ShellcheckComment): string { type LinterOptions = { executablePath: string | null + console: LSP.RemoteConsole cwd?: string } -export async function getLinterExecutablePath(): Promise { - return config.getShellcheckPath() || (await which('shellcheck')) -} - export class Linter { public executablePath: string | null private cwd: string + private console: LSP.RemoteConsole + _canLint: boolean - constructor(opts: LinterOptions) { - this.executablePath = opts.executablePath - this.cwd = opts.cwd || process.cwd() - this._canLint = !!this.executablePath + constructor({ console, cwd, executablePath }: LinterOptions) { + this.executablePath = executablePath + this.cwd = cwd || process.cwd() + this._canLint = !!this.executablePath // TODO: any reason to create a linter if the path is null? + this.console = console } public get canLint(): boolean { @@ -105,12 +102,14 @@ export class Linter { } 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.console.warn( + `ShellCheck: disabling linting as no executable '${this.executablePath}'`, + ) this._canLint = false return { comments: [] } } throw new Error( - `shellcheck failed with code ${exit}: ${e}\nout:\n${out}\nerr:\n${err}`, + `ShellCheck: failed with code ${exit}: ${e}\nout:\n${out}\nerr:\n${err}`, ) } @@ -119,7 +118,7 @@ export class Linter { raw = JSON.parse(out) } catch (e) { throw new Error( - `shellcheck: json parse failed with error ${e}\nout:\n${out}\nerr:\n${err}`, + `ShellCheck: json parse failed with error ${e}\nout:\n${out}\nerr:\n${err}`, ) } assertShellcheckResult(raw) diff --git a/server/src/server.ts b/server/src/server.ts index ee713047c..e638a181c 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -8,7 +8,7 @@ import Analyzer from './analyser' import * as Builtins from './builtins' import * as config from './config' import Executables from './executables' -import { getLinterExecutablePath, Linter } from './linter' +import { Linter } from './linter' import { initializeParser } from './parser' import * as ReservedWords from './reservedWords' import { BashCompletionItem, CompletionItemDataType } from './types' @@ -30,35 +30,41 @@ export default class BashServer { connection: LSP.Connection, { rootPath, capabilities }: LSP.InitializeParams, ): Promise { - const parser = await initializeParser() - const { PATH } = process.env if (!PATH) { throw new Error('Expected PATH environment variable to be set') } - return Promise.all([Executables.fromPath(PATH), getLinterExecutablePath()]).then( - ([executables, linterExecutablePath]) => { - const analyzer = new Analyzer({ console: connection.console, parser }) - const linter = new Linter({ executablePath: linterExecutablePath }) + const parser = await initializeParser() + const analyzer = new Analyzer({ console: connection.console, parser }) - let backgroundAnalysisCompleted - if (rootPath) { - // NOTE: we do not block the server initialization on this background analysis. - backgroundAnalysisCompleted = analyzer.initiateBackgroundAnalysis({ rootPath }) - } + let backgroundAnalysisCompleted + if (rootPath) { + // NOTE: we do not block the server initialization on this background analysis. + backgroundAnalysisCompleted = analyzer.initiateBackgroundAnalysis({ rootPath }) + } - return new BashServer({ - analyzer, - backgroundAnalysisCompleted, - capabilities, - connection, - executables, - linter, - }) - }, - ) + const shellcheckPath = config.getShellcheckPath() + const linter = new Linter({ + console: connection.console, + executablePath: shellcheckPath, + }) + + if (!shellcheckPath) { + connection.console.info('ShellCheck linting is disabled.') + } + + const executables = await Executables.fromPath(PATH) + + return new BashServer({ + analyzer, + backgroundAnalysisCompleted, + capabilities, + connection, + executables, + linter, + }) } private executables: Executables diff --git a/server/yarn.lock b/server/yarn.lock index 60a38117e..1f1a27dd3 100644 --- a/server/yarn.lock +++ b/server/yarn.lock @@ -161,11 +161,6 @@ is-number@^7.0.0: resolved "https://registry.yarnpkg.com/is-number/-/is-number-7.0.0.tgz#7535345b896734d5f80c4d06c50955527a14f12b" integrity sha512-41Cifkg6e8TylSpdtTpeLVMqvSBEVzTttHvERD741+pnZ8ANv0004MRL43QKPDlK9cGvNp6NZWZUBlbGXYxxng== -isexe@^2.0.0: - version "2.0.0" - resolved "https://registry.yarnpkg.com/isexe/-/isexe-2.0.0.tgz#e8fbf374dc556ff8947a10dcb0572d633f2cfa10" - integrity sha512-RHxMLp9lnKHGHRng9QFhRCMbYAcVpn69smSGcq3f36xjgVVWThj4qqLbTLlq7Ssj8B+fIQ1EuCEGI2lKsyQeIw== - merge2@^1.3.0: version "1.4.1" resolved "https://registry.yarnpkg.com/merge2/-/merge2-1.4.1.tgz#4368892f885e907455a6fd7dc55c0c9d404990ae" @@ -291,10 +286,3 @@ whatwg-url@^5.0.0: dependencies: tr46 "~0.0.3" webidl-conversions "^3.0.0" - -which@^2.0.2: - version "2.0.2" - resolved "https://registry.yarnpkg.com/which/-/which-2.0.2.tgz#7c6a8dd0a636a0327e10b59c9286eee93f3f51b1" - integrity sha512-BLI3Tl1TW3Pvl70l3yq3Y64i+awpwXqsGBYWkkqMtnbXgrMD+yj7rhW0kuEDxzJaYXGjEW5ogapKNMEKNMjibA== - dependencies: - isexe "^2.0.0" From 0b582a5a53c21f845a78de7a52d9b27676e0cd42 Mon Sep 17 00:00:00 2001 From: skovhus Date: Mon, 28 Nov 2022 10:18:52 +0100 Subject: [PATCH 2/3] Make the vscode extension default to use shellcheck --- vscode-client/package.json | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/vscode-client/package.json b/vscode-client/package.json index c34246d9f..3f0dd2dab 100644 --- a/vscode-client/package.json +++ b/vscode-client/package.json @@ -34,7 +34,7 @@ "bashIde.globPattern": { "type": "string", "default": "**/*@(.sh|.inc|.bash|.command)", - "description": "Glob pattern for finding and parsing shell script files." + "description": "Glob pattern for finding and parsing shell script files in the workspace." }, "bashIde.highlightParsingErrors": { "type": "boolean", @@ -44,12 +44,12 @@ "bashIde.explainshellEndpoint": { "type": "string", "default": "", - "description": "Configure explainshell server in order to get hover documentation on flags and options." + "description": "Configure explainshell server endpoint in order to get hover documentation on flags and options." }, "bashIde.shellcheckPath": { "type": "string", - "default": "", - "description": "Configure a custom path for the Shellcheck executable in order to get linting information." + "default": "shellcheck", + "description": "Controls the executable used for ShellCheck linting information. An empty string will disable linting." } } } From 11a39c9caed8cc404b9dededcfc4f66fa29ebee1 Mon Sep 17 00:00:00 2001 From: skovhus Date: Mon, 28 Nov 2022 10:27:44 +0100 Subject: [PATCH 3/3] Update test --- server/src/__tests__/linter.test.ts | 29 ++++++++++++++++++++--------- server/src/linter.ts | 2 +- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/server/src/__tests__/linter.test.ts b/server/src/__tests__/linter.test.ts index 5739da532..7fe83ac9e 100644 --- a/server/src/__tests__/linter.test.ts +++ b/server/src/__tests__/linter.test.ts @@ -2,31 +2,36 @@ import * as path from 'path' import * as LSP from 'vscode-languageserver' import { FIXTURE_DOCUMENT, FIXTURE_FOLDER } from '../../../testing/fixtures' +import { getMockConnection } from '../../../testing/mocks' import { assertShellcheckResult, Linter } from '../linter' +const mockConsole = getMockConnection().console + 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) + expect(new Linter({ console: mockConsole, executablePath: null }).canLint).toBe(false) }) it('should set canLint to true if executable not empty', () => { - expect(new Linter({ executablePath: 'foo' }).canLint).toBe(true) + expect(new Linter({ console: mockConsole, executablePath: 'foo' }).canLint).toBe(true) }) 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({ + console: mockConsole, executablePath, }) expect(await linter.lint(textToDoc(''), [])).toEqual([]) expect(linter.canLint).toBe(false) - expect(console.error).toBeCalledWith( - expect.stringContaining('shellcheck not available at path'), + expect(mockConsole.warn).toBeCalledWith( + expect.stringContaining( + 'ShellCheck: disabling linting as no executable was found at path', + ), ) }) @@ -54,21 +59,26 @@ describe('linter', () => { }, ] - const linter = new Linter({ executablePath: 'shellcheck' }) + const linter = new Linter({ console: mockConsole, 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 linter = new Linter({ + console: mockConsole, + 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', + console: mockConsole, cwd: path.resolve(path.join(FIXTURE_FOLDER, '../')), + executablePath: 'shellcheck', }) // prettier-ignore const expected = [ @@ -81,8 +91,9 @@ describe('linter', () => { it('should follow sources with incorrect cwd if correct path is passed as a workspace path', async () => { const linter = new Linter({ - executablePath: 'shellcheck', + console: mockConsole, cwd: path.resolve(path.join(FIXTURE_FOLDER, '../')), + executablePath: 'shellcheck', }) const result = await linter.lint(FIXTURE_DOCUMENT.SHELLCHECK_SOURCE, [ { uri: `file://${path.resolve(FIXTURE_FOLDER)}`, name: 'fixtures' }, diff --git a/server/src/linter.ts b/server/src/linter.ts index 9734eefc9..1da86393e 100644 --- a/server/src/linter.ts +++ b/server/src/linter.ts @@ -103,7 +103,7 @@ export class Linter { if ((e as any).code === 'ENOENT') { // shellcheck path wasn't found, don't try to lint any more: this.console.warn( - `ShellCheck: disabling linting as no executable '${this.executablePath}'`, + `ShellCheck: disabling linting as no executable was found at path '${this.executablePath}'`, ) this._canLint = false return { comments: [] }