From 04670bf685c3b4b97a5261923e7171abefe08d00 Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Wed, 27 Mar 2024 23:41:09 +0000 Subject: [PATCH 01/12] Add basic support for formatting using shfmt Limited to using shfmt's defaults for now - no way to tweak these. --- server/src/config.ts | 4 ++ server/src/server.ts | 35 ++++++++++++++ server/src/shfmt/index.ts | 98 ++++++++++++++++++++++++++++++++++++++ vscode-client/package.json | 5 ++ 4 files changed, 142 insertions(+) create mode 100644 server/src/shfmt/index.ts diff --git a/server/src/config.ts b/server/src/config.ts index c39b6413c..dde908f7b 100644 --- a/server/src/config.ts +++ b/server/src/config.ts @@ -40,6 +40,9 @@ export const ConfigSchema = z.object({ // Controls the executable used for ShellCheck linting information. An empty string will disable linting. shellcheckPath: z.string().trim().default('shellcheck'), + + // Controls the executable used for Shfmt formatting. An empty string will disable formatting + shfmtPath: z.string().trim().default('shfmt'), }) export type Config = z.infer @@ -57,6 +60,7 @@ export function getConfigFromEnvironmentVariables(): { logLevel: process.env[LOG_LEVEL_ENV_VAR], shellcheckArguments: process.env.SHELLCHECK_ARGUMENTS, shellcheckPath: process.env.SHELLCHECK_PATH, + shfmtPath: process.env.SHFMT_PATH, } const environmentVariablesUsed = Object.entries(rawConfig) diff --git a/server/src/server.ts b/server/src/server.ts index 8b70dc258..b8fce73de 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -13,6 +13,7 @@ import Executables from './executables' import { initializeParser } from './parser' import * as ReservedWords from './reserved-words' import { Linter, LintingResult } from './shellcheck' +import { Formatter } from './shfmt' import { SNIPPETS } from './snippets' import { BashCompletionItem, CompletionItemDataType } from './types' import { uniqueBasedOnHash } from './util/array' @@ -35,6 +36,7 @@ export default class BashServer { private documents: LSP.TextDocuments = new LSP.TextDocuments(TextDocument) private executables: Executables private linter?: Linter + private formatter?: Formatter private workspaceFolder: string | null private uriToCodeActions: { [uri: string]: LintingResult['codeActions'] | undefined @@ -46,6 +48,7 @@ export default class BashServer { connection, executables, linter, + formatter, workspaceFolder, }: { analyzer: Analyzer @@ -53,6 +56,7 @@ export default class BashServer { connection: LSP.Connection executables: Executables linter?: Linter + formatter?: Formatter workspaceFolder: string | null }) { this.analyzer = analyzer @@ -60,6 +64,7 @@ export default class BashServer { this.connection = connection this.executables = executables this.linter = linter + this.formatter = formatter this.workspaceFolder = workspaceFolder this.config = {} as any // NOTE: configured in updateConfiguration this.updateConfiguration(config.getDefaultConfiguration(), true) @@ -130,6 +135,7 @@ export default class BashServer { workDoneProgress: false, }, renameProvider: { prepareProvider: true }, + documentFormattingProvider: true, } } @@ -172,6 +178,7 @@ export default class BashServer { connection.onWorkspaceSymbol(this.onWorkspaceSymbol.bind(this)) connection.onPrepareRename(this.onPrepareRename.bind(this)) connection.onRenameRequest(this.onRenameRequest.bind(this)) + connection.onDocumentFormatting(this.onDocumentFormatting.bind(this)) /** * The initialized notification is sent from the client to the server after @@ -272,6 +279,14 @@ export default class BashServer { this.linter = new Linter({ executablePath: shellcheckPath }) } + const { shfmtPath } = this.config + if (!shfmtPath) { + logger.info('Shfmt formatting is disabled as "shfmtPath" was not set') + this.formatter = undefined + } else { + this.formatter = new Formatter({ executablePath: shfmtPath }) + } + this.analyzer.setEnableSourceErrorDiagnostics( this.config.enableSourceErrorDiagnostics, ) @@ -806,6 +821,26 @@ export default class BashServer { } return edits } + + private async onDocumentFormatting( + params: LSP.DocumentFormattingParams, + ): Promise { + if (this.formatter) { + try { + const document = this.documents.get(params.textDocument.uri) + if (!document) { + logger.error(`Error getting document: ${params.textDocument.uri}`) + return null + } + + return await this.formatter.format(document) + } catch (err) { + logger.error(`Error while formatting: ${err}`) + } + } + + return null + } } /** diff --git a/server/src/shfmt/index.ts b/server/src/shfmt/index.ts new file mode 100644 index 000000000..05afd5c96 --- /dev/null +++ b/server/src/shfmt/index.ts @@ -0,0 +1,98 @@ +import { spawn } from 'child_process' +import * as LSP from 'vscode-languageserver/node' +import { TextDocument, TextEdit } from 'vscode-languageserver-textdocument' + +import { logger } from '../util/logger' + +type FormatterOptions = { + executablePath: string + cwd?: string +} + +export class Formatter { + private cwd: string + public executablePath: string + private _canFormat: boolean + + constructor({ cwd, executablePath }: FormatterOptions) { + this._canFormat = true + this.cwd = cwd || process.cwd() + this.executablePath = executablePath + } + + public get canFormat(): boolean { + return this._canFormat + } + + public async format(document: TextDocument): Promise { + if (!this._canFormat) { + return [] + } + + return this.executeFormat(document) + } + + private async executeFormat(document: TextDocument): Promise { + const documentText = document.getText() + + const result = await this.runShfmt(documentText) + + if (!this._canFormat) { + return [] + } + + return [ + { + range: LSP.Range.create( + LSP.Position.create(0, 0), + LSP.Position.create(Number.MAX_VALUE, Number.MAX_VALUE), + ), + newText: result, + }, + ] + } + + private async runShfmt(documentText: string): Promise { + const args: string[] = [] + + logger.debug(`Shfmt: running "${this.executablePath} ${args.join(' ')}"`) + + let out = '' + let err = '' + const proc = new Promise((resolve, reject) => { + const proc = spawn(this.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', () => { + // NOTE: 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(documentText) + }) + + // NOTE: 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) { + // TODO: we could do this up front? + if ((e as any).code === 'ENOENT') { + // shfmt path wasn't found, don't try to format any more: + logger.warn( + `Shfmt: disabling formatting as no executable was found at path '${this.executablePath}'`, + ) + this._canFormat = false + return '' + } + throw new Error(`Shfmt: failed with code ${exit}: ${e}\nout:\n${out}\nerr:\n${err}`) + } + + return out + } +} diff --git a/vscode-client/package.json b/vscode-client/package.json index fd17f0550..d81829df9 100644 --- a/vscode-client/package.json +++ b/vscode-client/package.json @@ -77,6 +77,11 @@ "type": "string", "default": "", "description": "Additional ShellCheck arguments. Note that we already add the following arguments: --shell, --format, --external-sources." + }, + "bashIde.shfmtPath": { + "type": "string", + "default": "shfmt", + "description": "Controls the executable used for Shfmt formatting. An empty string will disable formatting." } } } From e1c0a2a358e76cc26c69de19699b4a94b72a5964 Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Thu, 28 Mar 2024 23:39:34 +0000 Subject: [PATCH 02/12] Add tests for basic formatting with shfmt --- server/src/__tests__/config.test.ts | 8 ++ server/src/__tests__/server.test.ts | 1 + server/src/shfmt/__tests__/index.test.ts | 93 ++++++++++++++++++++++++ testing/fixtures.ts | 1 + testing/fixtures/shfmt.sh | 21 ++++++ 5 files changed, 124 insertions(+) create mode 100644 server/src/shfmt/__tests__/index.test.ts create mode 100644 testing/fixtures/shfmt.sh diff --git a/server/src/__tests__/config.test.ts b/server/src/__tests__/config.test.ts index a83526a0d..0a7a7a2b1 100644 --- a/server/src/__tests__/config.test.ts +++ b/server/src/__tests__/config.test.ts @@ -13,6 +13,7 @@ describe('ConfigSchema', () => { "logLevel": "info", "shellcheckArguments": [], "shellcheckPath": "shellcheck", + "shfmtPath": "shfmt", } `) }) @@ -25,6 +26,7 @@ describe('ConfigSchema', () => { includeAllWorkspaceSymbols: true, shellcheckArguments: ' -e SC2001 -e SC2002 ', shellcheckPath: '', + shfmtPath: 'myshfmt', }), ).toMatchInlineSnapshot(` { @@ -41,6 +43,7 @@ describe('ConfigSchema', () => { "SC2002", ], "shellcheckPath": "", + "shfmtPath": "myshfmt", } `) }) @@ -67,12 +70,14 @@ describe('getConfigFromEnvironmentVariables', () => { "logLevel": "info", "shellcheckArguments": [], "shellcheckPath": "shellcheck", + "shfmtPath": "shfmt", } `) }) it('preserves an empty string', () => { process.env = { SHELLCHECK_PATH: '', + SHFMT_PATH: '', EXPLAINSHELL_ENDPOINT: '', } const { config } = getConfigFromEnvironmentVariables() @@ -86,6 +91,7 @@ describe('getConfigFromEnvironmentVariables', () => { "logLevel": "info", "shellcheckArguments": [], "shellcheckPath": "", + "shfmtPath": "", } `) }) @@ -94,6 +100,7 @@ describe('getConfigFromEnvironmentVariables', () => { process.env = { SHELLCHECK_PATH: '/path/to/shellcheck', SHELLCHECK_ARGUMENTS: '-e SC2001', + SHFMT_PATH: '/path/to/shfmt', EXPLAINSHELL_ENDPOINT: 'localhost:8080', GLOB_PATTERN: '*.*', BACKGROUND_ANALYSIS_MAX_FILES: '1', @@ -113,6 +120,7 @@ describe('getConfigFromEnvironmentVariables', () => { "SC2001", ], "shellcheckPath": "/path/to/shellcheck", + "shfmtPath": "/path/to/shfmt", } `) }) diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts index e6b88d66c..7c10dba82 100644 --- a/server/src/__tests__/server.test.ts +++ b/server/src/__tests__/server.test.ts @@ -85,6 +85,7 @@ describe('server', () => { ], }, "definitionProvider": true, + "documentFormattingProvider": true, "documentHighlightProvider": true, "documentSymbolProvider": true, "hoverProvider": true, diff --git a/server/src/shfmt/__tests__/index.test.ts b/server/src/shfmt/__tests__/index.test.ts new file mode 100644 index 000000000..4e920491d --- /dev/null +++ b/server/src/shfmt/__tests__/index.test.ts @@ -0,0 +1,93 @@ +import { TextDocument } from 'vscode-languageserver-textdocument' + +import { FIXTURE_DOCUMENT, FIXTURE_FOLDER } from '../../../../testing/fixtures' +import { Logger } from '../../util/logger' +import { Formatter } from '../index' + +jest.spyOn(Logger.prototype, 'log').mockImplementation(() => { + // noop +}) +const loggerWarn = jest.spyOn(Logger.prototype, 'warn') + +const FIXTURE_DOCUMENT_URI = `file://${FIXTURE_FOLDER}/foo.sh` +function textToDoc(txt: string) { + return TextDocument.create(FIXTURE_DOCUMENT_URI, 'bar', 0, txt) +} + +async function getFormattingResult({ + document, + executablePath = 'shfmt', +}: { + document: TextDocument + executablePath?: string +}): Promise<[Awaited>, Formatter]> { + const formatter = new Formatter({ + executablePath, + }) + const result = await formatter.format(document) + return [result, formatter] +} + +describe('formatter', () => { + it('defaults canFormat to true', () => { + expect(new Formatter({ executablePath: 'foo' }).canFormat).toBe(true) + }) + + it('should set canFormat to false when formatting fails', async () => { + const [result, formatter] = await getFormattingResult({ + document: textToDoc(''), + executablePath: 'foo', + }) + + expect(result).toEqual([]) + + expect(formatter.canFormat).toBe(false) + expect(loggerWarn).toBeCalledWith( + expect.stringContaining( + 'Shfmt: disabling formatting as no executable was found at path', + ), + ) + }) + + it('should format when shfmt is present', async () => { + const [result] = await getFormattingResult({ document: FIXTURE_DOCUMENT.SHFMT }) + expect(result).toMatchInlineSnapshot(` + [ + { + "newText": "#!/bin/bash + set -ueo pipefail + + if [ -z "$arg" ]; then + echo indent + fi + + echo binary && + echo next line + + case "$arg" in + a) + echo case indent + ;; + esac + + echo space redirects >/dev/null + + function next() { + echo line + } + ", + "range": { + "end": { + "character": 2147483647, + "line": 2147483647, + }, + "start": { + "character": 0, + "line": 0, + }, + }, + }, + ] + `) + }) +}) diff --git a/testing/fixtures.ts b/testing/fixtures.ts index 009174836..2bee6ef8f 100644 --- a/testing/fixtures.ts +++ b/testing/fixtures.ts @@ -34,6 +34,7 @@ export const FIXTURE_URI = { 'shellcheck', 'shell-directive.bash', )}`, + SHFMT: `file://${path.join(FIXTURE_FOLDER, 'shfmt.sh')}`, SOURCING: `file://${path.join(FIXTURE_FOLDER, 'sourcing.sh')}`, SOURCING2: `file://${path.join(FIXTURE_FOLDER, 'sourcing2.sh')}`, RENAMING: `file://${path.join(FIXTURE_FOLDER, 'renaming.sh')}`, diff --git a/testing/fixtures/shfmt.sh b/testing/fixtures/shfmt.sh new file mode 100644 index 000000000..ff14200f8 --- /dev/null +++ b/testing/fixtures/shfmt.sh @@ -0,0 +1,21 @@ +#!/bin/bash +set -ueo pipefail + +if [ -z "$arg" ]; then +echo indent +fi + +echo binary&& +echo next line + +case "$arg" in +a) +echo case indent +;; +esac + +echo space redirects> /dev/null + +function next(){ +echo line +} From 12db6714b0a80ac3235360f52cb70a4af4a669ba Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Fri, 29 Mar 2024 00:18:40 +0000 Subject: [PATCH 03/12] Respect editor config for tab size when formatting If the editor config is set to insert spaces instead of tabs, shfmt will respect that choice (including the number of spaces to insert). --- server/src/server.ts | 2 +- server/src/shfmt/__tests__/index.test.ts | 95 +++++++++++++++++++++++- server/src/shfmt/index.ts | 22 ++++-- 3 files changed, 111 insertions(+), 8 deletions(-) diff --git a/server/src/server.ts b/server/src/server.ts index b8fce73de..00c735c4c 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -833,7 +833,7 @@ export default class BashServer { return null } - return await this.formatter.format(document) + return await this.formatter.format(document, params.options) } catch (err) { logger.error(`Error while formatting: ${err}`) } diff --git a/server/src/shfmt/__tests__/index.test.ts b/server/src/shfmt/__tests__/index.test.ts index 4e920491d..af111fd5c 100644 --- a/server/src/shfmt/__tests__/index.test.ts +++ b/server/src/shfmt/__tests__/index.test.ts @@ -1,3 +1,4 @@ +import { FormattingOptions } from 'vscode-languageserver/node' import { TextDocument } from 'vscode-languageserver-textdocument' import { FIXTURE_DOCUMENT, FIXTURE_FOLDER } from '../../../../testing/fixtures' @@ -17,14 +18,16 @@ function textToDoc(txt: string) { async function getFormattingResult({ document, executablePath = 'shfmt', + formatOptions, }: { document: TextDocument executablePath?: string + formatOptions?: FormattingOptions }): Promise<[Awaited>, Formatter]> { const formatter = new Formatter({ executablePath, }) - const result = await formatter.format(document) + const result = await formatter.format(document, formatOptions) return [result, formatter] } @@ -90,4 +93,94 @@ describe('formatter', () => { ] `) }) + + it('should format using tabs when insertSpaces is false', async () => { + const [result] = await getFormattingResult({ + document: FIXTURE_DOCUMENT.SHFMT, + formatOptions: { tabSize: 4, insertSpaces: false }, + }) + expect(result).toMatchInlineSnapshot(` + [ + { + "newText": "#!/bin/bash + set -ueo pipefail + + if [ -z "$arg" ]; then + echo indent + fi + + echo binary && + echo next line + + case "$arg" in + a) + echo case indent + ;; + esac + + echo space redirects >/dev/null + + function next() { + echo line + } + ", + "range": { + "end": { + "character": 2147483647, + "line": 2147483647, + }, + "start": { + "character": 0, + "line": 0, + }, + }, + }, + ] + `) + }) + + it('should format using spaces when insertSpaces is true', async () => { + const [result] = await getFormattingResult({ + document: FIXTURE_DOCUMENT.SHFMT, + formatOptions: { tabSize: 3, insertSpaces: true }, + }) + expect(result).toMatchInlineSnapshot(` + [ + { + "newText": "#!/bin/bash + set -ueo pipefail + + if [ -z "$arg" ]; then + echo indent + fi + + echo binary && + echo next line + + case "$arg" in + a) + echo case indent + ;; + esac + + echo space redirects >/dev/null + + function next() { + echo line + } + ", + "range": { + "end": { + "character": 2147483647, + "line": 2147483647, + }, + "start": { + "character": 0, + "line": 0, + }, + }, + }, + ] + `) + }) }) diff --git a/server/src/shfmt/index.ts b/server/src/shfmt/index.ts index 05afd5c96..906a2126a 100644 --- a/server/src/shfmt/index.ts +++ b/server/src/shfmt/index.ts @@ -24,18 +24,24 @@ export class Formatter { return this._canFormat } - public async format(document: TextDocument): Promise { + public async format( + document: TextDocument, + formatOptions?: LSP.FormattingOptions | null, + ): Promise { if (!this._canFormat) { return [] } - return this.executeFormat(document) + return this.executeFormat(document, formatOptions) } - private async executeFormat(document: TextDocument): Promise { + private async executeFormat( + document: TextDocument, + formatOptions?: LSP.FormattingOptions | null, + ): Promise { const documentText = document.getText() - const result = await this.runShfmt(documentText) + const result = await this.runShfmt(documentText, formatOptions) if (!this._canFormat) { return [] @@ -52,8 +58,12 @@ export class Formatter { ] } - private async runShfmt(documentText: string): Promise { - const args: string[] = [] + private async runShfmt( + documentText: string, + formatOptions?: LSP.FormattingOptions | null, + ): Promise { + const indentation: number = formatOptions?.insertSpaces ? formatOptions.tabSize : 0 + const args: string[] = [`--indent=${indentation}`] logger.debug(`Shfmt: running "${this.executablePath} ${args.join(' ')}"`) From 2a4e81d6df3c0782cb327442bb50fecaca8d1fda Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Sat, 30 Mar 2024 00:14:20 +0000 Subject: [PATCH 04/12] Fix tests following addition of shfmt.sh fixture --- server/src/__tests__/__snapshots__/server.test.ts.snap | 2 ++ server/src/__tests__/analyzer.test.ts | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/server/src/__tests__/__snapshots__/server.test.ts.snap b/server/src/__tests__/__snapshots__/server.test.ts.snap index 87f13902a..e9266b0e1 100644 --- a/server/src/__tests__/__snapshots__/server.test.ts.snap +++ b/server/src/__tests__/__snapshots__/server.test.ts.snap @@ -909,6 +909,7 @@ exports[`server onRenameRequest Workspace-wide rename returns correct WorkspaceE "file://__REPO_ROOT_FOLDER__/testing/fixtures/shellcheck/shell-directive.bash": [], "file://__REPO_ROOT_FOLDER__/testing/fixtures/shellcheck/source.sh": [], "file://__REPO_ROOT_FOLDER__/testing/fixtures/shellcheck/sourced.sh": [], + "file://__REPO_ROOT_FOLDER__/testing/fixtures/shfmt.sh": [], "file://__REPO_ROOT_FOLDER__/testing/fixtures/sourcing.sh": [], "file://__REPO_ROOT_FOLDER__/testing/fixtures/sourcing2.sh": [], }, @@ -1001,6 +1002,7 @@ exports[`server onRenameRequest Workspace-wide rename returns correct WorkspaceE "file://__REPO_ROOT_FOLDER__/testing/fixtures/shellcheck/shell-directive.bash": [], "file://__REPO_ROOT_FOLDER__/testing/fixtures/shellcheck/source.sh": [], "file://__REPO_ROOT_FOLDER__/testing/fixtures/shellcheck/sourced.sh": [], + "file://__REPO_ROOT_FOLDER__/testing/fixtures/shfmt.sh": [], "file://__REPO_ROOT_FOLDER__/testing/fixtures/sourcing.sh": [], "file://__REPO_ROOT_FOLDER__/testing/fixtures/sourcing2.sh": [], }, diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index f8e4c0320..8d4aa357e 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/server/src/__tests__/analyzer.test.ts @@ -16,7 +16,7 @@ import { Logger } from '../util/logger' const CURRENT_URI = 'dummy-uri.sh' // if you add a .sh file to testing/fixtures, update this value -const FIXTURE_FILES_MATCHING_GLOB = 18 +const FIXTURE_FILES_MATCHING_GLOB = 19 const defaultConfig = getDefaultConfiguration() From 5bf17953ba50292917488e570c0d7dfe704a6a57 Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Sat, 30 Mar 2024 01:44:11 +0000 Subject: [PATCH 05/12] Add config params for shfmt printer flags --- server/src/__tests__/config.test.ts | 49 ++++++++++++++++++++++---- server/src/config.ts | 28 +++++++++++++-- server/src/server.ts | 4 +-- vscode-client/__tests__/config.test.ts | 14 +++++++- vscode-client/package.json | 22 +++++++++++- 5 files changed, 104 insertions(+), 13 deletions(-) diff --git a/server/src/__tests__/config.test.ts b/server/src/__tests__/config.test.ts index 0a7a7a2b1..1d02dcf00 100644 --- a/server/src/__tests__/config.test.ts +++ b/server/src/__tests__/config.test.ts @@ -13,7 +13,13 @@ describe('ConfigSchema', () => { "logLevel": "info", "shellcheckArguments": [], "shellcheckPath": "shellcheck", - "shfmtPath": "shfmt", + "shfmt": { + "binaryNextLine": false, + "caseIndent": false, + "funcNextLine": false, + "path": "shfmt", + "spaceRedirects": false, + }, } `) }) @@ -26,7 +32,13 @@ describe('ConfigSchema', () => { includeAllWorkspaceSymbols: true, shellcheckArguments: ' -e SC2001 -e SC2002 ', shellcheckPath: '', - shfmtPath: 'myshfmt', + shfmt: { + binaryNextLine: true, + caseIndent: true, + funcNextLine: true, + path: 'myshfmt', + spaceRedirects: true, + }, }), ).toMatchInlineSnapshot(` { @@ -43,7 +55,13 @@ describe('ConfigSchema', () => { "SC2002", ], "shellcheckPath": "", - "shfmtPath": "myshfmt", + "shfmt": { + "binaryNextLine": true, + "caseIndent": true, + "funcNextLine": true, + "path": "myshfmt", + "spaceRedirects": true, + }, } `) }) @@ -70,7 +88,13 @@ describe('getConfigFromEnvironmentVariables', () => { "logLevel": "info", "shellcheckArguments": [], "shellcheckPath": "shellcheck", - "shfmtPath": "shfmt", + "shfmt": { + "binaryNextLine": false, + "caseIndent": false, + "funcNextLine": false, + "path": "shfmt", + "spaceRedirects": false, + }, } `) }) @@ -91,7 +115,13 @@ describe('getConfigFromEnvironmentVariables', () => { "logLevel": "info", "shellcheckArguments": [], "shellcheckPath": "", - "shfmtPath": "", + "shfmt": { + "binaryNextLine": false, + "caseIndent": false, + "funcNextLine": false, + "path": "", + "spaceRedirects": false, + }, } `) }) @@ -101,6 +131,7 @@ describe('getConfigFromEnvironmentVariables', () => { SHELLCHECK_PATH: '/path/to/shellcheck', SHELLCHECK_ARGUMENTS: '-e SC2001', SHFMT_PATH: '/path/to/shfmt', + SHFMT_CASE_INDENT: 'true', EXPLAINSHELL_ENDPOINT: 'localhost:8080', GLOB_PATTERN: '*.*', BACKGROUND_ANALYSIS_MAX_FILES: '1', @@ -120,7 +151,13 @@ describe('getConfigFromEnvironmentVariables', () => { "SC2001", ], "shellcheckPath": "/path/to/shellcheck", - "shfmtPath": "/path/to/shfmt", + "shfmt": { + "binaryNextLine": false, + "caseIndent": true, + "funcNextLine": false, + "path": "/path/to/shfmt", + "spaceRedirects": false, + }, } `) }) diff --git a/server/src/config.ts b/server/src/config.ts index dde908f7b..1e39903b7 100644 --- a/server/src/config.ts +++ b/server/src/config.ts @@ -41,8 +41,24 @@ export const ConfigSchema = z.object({ // Controls the executable used for ShellCheck linting information. An empty string will disable linting. shellcheckPath: z.string().trim().default('shellcheck'), - // Controls the executable used for Shfmt formatting. An empty string will disable formatting - shfmtPath: z.string().trim().default('shfmt'), + shfmt: z + .object({ + // Controls the executable used for Shfmt formatting. An empty string will disable formatting + path: z.string().trim().default('shfmt'), + + // Allow boolean operators (like && and ||) to start a line. + binaryNextLine: z.boolean().default(false), + + // Indent patterns in case statements. + caseIndent: z.boolean().default(false), + + // Place function opening braces on a separate line. + funcNextLine: z.boolean().default(false), + + // Follow redirection operators with a space. + spaceRedirects: z.boolean().default(false), + }) + .default({}), }) export type Config = z.infer @@ -60,7 +76,13 @@ export function getConfigFromEnvironmentVariables(): { logLevel: process.env[LOG_LEVEL_ENV_VAR], shellcheckArguments: process.env.SHELLCHECK_ARGUMENTS, shellcheckPath: process.env.SHELLCHECK_PATH, - shfmtPath: process.env.SHFMT_PATH, + shfmt: { + path: process.env.SHFMT_PATH, + binaryNextLine: toBoolean(process.env.SHFMT_BINARY_NEXT_LINE), + caseIndent: toBoolean(process.env.SHFMT_CASE_INDENT), + funcNextLine: toBoolean(process.env.SHFMT_FUNC_NEXT_LINE), + spaceRedirects: toBoolean(process.env.SHFMT_SPACE_REDIRECTS), + }, } const environmentVariablesUsed = Object.entries(rawConfig) diff --git a/server/src/server.ts b/server/src/server.ts index 00c735c4c..19e504b98 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -279,9 +279,9 @@ export default class BashServer { this.linter = new Linter({ executablePath: shellcheckPath }) } - const { shfmtPath } = this.config + const shfmtPath = this.config.shfmt?.path if (!shfmtPath) { - logger.info('Shfmt formatting is disabled as "shfmtPath" was not set') + logger.info('Shfmt formatting is disabled as "shfmt.path" was not set') this.formatter = undefined } else { this.formatter = new Formatter({ executablePath: shfmtPath }) diff --git a/vscode-client/__tests__/config.test.ts b/vscode-client/__tests__/config.test.ts index d1c753fb7..d6dfea9ab 100644 --- a/vscode-client/__tests__/config.test.ts +++ b/vscode-client/__tests__/config.test.ts @@ -4,6 +4,18 @@ import { LOG_LEVELS } from '../../server/src/util/logger' const defaultConfig = getDefaultConfiguration() +function flattenObjectKeys(obj: Record, prefix = '') { + return Object.keys(obj).reduce((flattenedKeys: string[], key) => { + const pre = prefix.length ? `${prefix}.` : '' + if (typeof obj[key] === 'object' && obj[key] !== null && !Array.isArray(obj[key])) { + flattenedKeys.push(...flattenObjectKeys(obj[key], pre + key)) + } else { + flattenedKeys.push(pre + key) + } + return flattenedKeys + }, []) +} + describe('config', () => { const configProperties = packageJson.contributes.configuration.properties @@ -18,7 +30,7 @@ describe('config', () => { .map((key) => key.replace(/^bashIde\./, '')) .sort() - const defaultConfigKeys = Object.keys(defaultConfig).sort() + const defaultConfigKeys = flattenObjectKeys(defaultConfig).sort() expect(configKeys).toEqual(defaultConfigKeys) }) diff --git a/vscode-client/package.json b/vscode-client/package.json index d81829df9..3cedc2e74 100644 --- a/vscode-client/package.json +++ b/vscode-client/package.json @@ -78,10 +78,30 @@ "default": "", "description": "Additional ShellCheck arguments. Note that we already add the following arguments: --shell, --format, --external-sources." }, - "bashIde.shfmtPath": { + "bashIde.shfmt.path": { "type": "string", "default": "shfmt", "description": "Controls the executable used for Shfmt formatting. An empty string will disable formatting." + }, + "bashIde.shfmt.binaryNextLine": { + "type": "boolean", + "default": "false", + "description": "Allow boolean operators (like && and ||) to start a line." + }, + "bashIde.shfmt.caseIndent": { + "type": "boolean", + "default": "false", + "description": "Indent patterns in case statements." + }, + "bashIde.shfmt.funcNextLine": { + "type": "boolean", + "default": "false", + "description": "Place function opening braces on a separate line." + }, + "bashIde.shfmt.spaceRedirects": { + "type": "boolean", + "default": "false", + "description": "Follow redirection operators with a space." } } } From cb63cb381a88c3dccc32f0dcce7fb1b10ca8b2b1 Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Sat, 30 Mar 2024 15:12:58 +0000 Subject: [PATCH 06/12] Add support for shfmt printer flags Add support for the following `shfmt` printer flags to control formatting: * `-bn`, `--binary-next-line` * `-ci`, `--case-indent` * `-fn`, `--func-next-line` * `-sr`, `--space-redirects` The `-i`, `--indent` flag is set appropriately based upon editor formatting options. Support for `-kp`, `--keep-padding` was not added as it has been deprecated and will be removed in the next major release of `shfmt`. --- server/src/server.ts | 2 +- server/src/shfmt/__tests__/index.test.ts | 241 ++++++++++++++++++++++- server/src/shfmt/index.ts | 11 +- 3 files changed, 250 insertions(+), 4 deletions(-) diff --git a/server/src/server.ts b/server/src/server.ts index 19e504b98..4e5f0c8df 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -833,7 +833,7 @@ export default class BashServer { return null } - return await this.formatter.format(document, params.options) + return await this.formatter.format(document, params.options, this.config.shfmt) } catch (err) { logger.error(`Error while formatting: ${err}`) } diff --git a/server/src/shfmt/__tests__/index.test.ts b/server/src/shfmt/__tests__/index.test.ts index af111fd5c..3bc3e3c3a 100644 --- a/server/src/shfmt/__tests__/index.test.ts +++ b/server/src/shfmt/__tests__/index.test.ts @@ -19,15 +19,17 @@ async function getFormattingResult({ document, executablePath = 'shfmt', formatOptions, + shfmtConfig, }: { document: TextDocument executablePath?: string formatOptions?: FormattingOptions + shfmtConfig?: Record }): Promise<[Awaited>, Formatter]> { const formatter = new Formatter({ executablePath, }) - const result = await formatter.format(document, formatOptions) + const result = await formatter.format(document, formatOptions, shfmtConfig) return [result, formatter] } @@ -183,4 +185,241 @@ describe('formatter', () => { ] `) }) + + it('should format with operators at the start of the line when binaryNextLine is true', async () => { + const [result] = await getFormattingResult({ + document: FIXTURE_DOCUMENT.SHFMT, + formatOptions: { tabSize: 2, insertSpaces: true }, + shfmtConfig: { binaryNextLine: true }, + }) + expect(result).toMatchInlineSnapshot(` + [ + { + "newText": "#!/bin/bash + set -ueo pipefail + + if [ -z "$arg" ]; then + echo indent + fi + + echo binary \\ + && echo next line + + case "$arg" in + a) + echo case indent + ;; + esac + + echo space redirects >/dev/null + + function next() { + echo line + } + ", + "range": { + "end": { + "character": 2147483647, + "line": 2147483647, + }, + "start": { + "character": 0, + "line": 0, + }, + }, + }, + ] + `) + }) + + it('should format with case patterns indented when caseIndent is true', async () => { + const [result] = await getFormattingResult({ + document: FIXTURE_DOCUMENT.SHFMT, + formatOptions: { tabSize: 2, insertSpaces: true }, + shfmtConfig: { caseIndent: true }, + }) + expect(result).toMatchInlineSnapshot(` + [ + { + "newText": "#!/bin/bash + set -ueo pipefail + + if [ -z "$arg" ]; then + echo indent + fi + + echo binary && + echo next line + + case "$arg" in + a) + echo case indent + ;; + esac + + echo space redirects >/dev/null + + function next() { + echo line + } + ", + "range": { + "end": { + "character": 2147483647, + "line": 2147483647, + }, + "start": { + "character": 0, + "line": 0, + }, + }, + }, + ] + `) + }) + + it('should format with function opening braces on a separate line when funcNextLine is true', async () => { + const [result] = await getFormattingResult({ + document: FIXTURE_DOCUMENT.SHFMT, + formatOptions: { tabSize: 2, insertSpaces: true }, + shfmtConfig: { funcNextLine: true }, + }) + expect(result).toMatchInlineSnapshot(` + [ + { + "newText": "#!/bin/bash + set -ueo pipefail + + if [ -z "$arg" ]; then + echo indent + fi + + echo binary && + echo next line + + case "$arg" in + a) + echo case indent + ;; + esac + + echo space redirects >/dev/null + + function next() + { + echo line + } + ", + "range": { + "end": { + "character": 2147483647, + "line": 2147483647, + }, + "start": { + "character": 0, + "line": 0, + }, + }, + }, + ] + `) + }) + + it('should format with redirect operators followed by a space when spaceRedirects is true', async () => { + const [result] = await getFormattingResult({ + document: FIXTURE_DOCUMENT.SHFMT, + formatOptions: { tabSize: 2, insertSpaces: true }, + shfmtConfig: { spaceRedirects: true }, + }) + expect(result).toMatchInlineSnapshot(` + [ + { + "newText": "#!/bin/bash + set -ueo pipefail + + if [ -z "$arg" ]; then + echo indent + fi + + echo binary && + echo next line + + case "$arg" in + a) + echo case indent + ;; + esac + + echo space redirects > /dev/null + + function next() { + echo line + } + ", + "range": { + "end": { + "character": 2147483647, + "line": 2147483647, + }, + "start": { + "character": 0, + "line": 0, + }, + }, + }, + ] + `) + }) + + it('should format with all options enabled when multiple config settings are combined', async () => { + const [result] = await getFormattingResult({ + document: FIXTURE_DOCUMENT.SHFMT, + formatOptions: { tabSize: 2, insertSpaces: true }, + shfmtConfig: { + binaryNextLine: true, + caseIndent: true, + funcNextLine: true, + spaceRedirects: true, + }, + }) + expect(result).toMatchInlineSnapshot(` + [ + { + "newText": "#!/bin/bash + set -ueo pipefail + + if [ -z "$arg" ]; then + echo indent + fi + + echo binary \\ + && echo next line + + case "$arg" in + a) + echo case indent + ;; + esac + + echo space redirects > /dev/null + + function next() + { + echo line + } + ", + "range": { + "end": { + "character": 2147483647, + "line": 2147483647, + }, + "start": { + "character": 0, + "line": 0, + }, + }, + }, + ] + `) + }) }) diff --git a/server/src/shfmt/index.ts b/server/src/shfmt/index.ts index 906a2126a..e6873a881 100644 --- a/server/src/shfmt/index.ts +++ b/server/src/shfmt/index.ts @@ -27,21 +27,23 @@ export class Formatter { public async format( document: TextDocument, formatOptions?: LSP.FormattingOptions | null, + shfmtConfig?: Record | null, ): Promise { if (!this._canFormat) { return [] } - return this.executeFormat(document, formatOptions) + return this.executeFormat(document, formatOptions, shfmtConfig) } private async executeFormat( document: TextDocument, formatOptions?: LSP.FormattingOptions | null, + shfmtConfig?: Record | null, ): Promise { const documentText = document.getText() - const result = await this.runShfmt(documentText, formatOptions) + const result = await this.runShfmt(documentText, formatOptions, shfmtConfig) if (!this._canFormat) { return [] @@ -61,9 +63,14 @@ export class Formatter { private async runShfmt( documentText: string, formatOptions?: LSP.FormattingOptions | null, + shfmtConfig?: Record | null, ): Promise { const indentation: number = formatOptions?.insertSpaces ? formatOptions.tabSize : 0 const args: string[] = [`--indent=${indentation}`] + if (shfmtConfig?.binaryNextLine) args.push('--binary-next-line') + if (shfmtConfig?.caseIndent) args.push('--case-indent') + if (shfmtConfig?.funcNextLine) args.push('--func-next-line') + if (shfmtConfig?.spaceRedirects) args.push('--space-redirects') logger.debug(`Shfmt: running "${this.executablePath} ${args.join(' ')}"`) From 57e0c4a372bb00bb202ae74398ec61b8e9e4058e Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Sat, 30 Mar 2024 15:28:04 +0000 Subject: [PATCH 07/12] Fix default values for shfmt config params The booleans should be booleans, not strings... --- 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 3cedc2e74..ded885c5d 100644 --- a/vscode-client/package.json +++ b/vscode-client/package.json @@ -85,22 +85,22 @@ }, "bashIde.shfmt.binaryNextLine": { "type": "boolean", - "default": "false", + "default": false, "description": "Allow boolean operators (like && and ||) to start a line." }, "bashIde.shfmt.caseIndent": { "type": "boolean", - "default": "false", + "default": false, "description": "Indent patterns in case statements." }, "bashIde.shfmt.funcNextLine": { "type": "boolean", - "default": "false", + "default": false, "description": "Place function opening braces on a separate line." }, "bashIde.shfmt.spaceRedirects": { "type": "boolean", - "default": "false", + "default": false, "description": "Follow redirection operators with a space." } } From 6eb243e45a10e8b6496161c904fb5b17bbb1fdcf Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Sat, 30 Mar 2024 15:36:12 +0000 Subject: [PATCH 08/12] Fix deactivate() for vscode-client The deactivate function was throwing exceptions as the `client` var didn't always have a value. Handle this by checking the var and returning undefined if it doesn't exist. --- vscode-client/src/extension.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/vscode-client/src/extension.ts b/vscode-client/src/extension.ts index 2f2ab3af9..dd65470f9 100644 --- a/vscode-client/src/extension.ts +++ b/vscode-client/src/extension.ts @@ -70,6 +70,9 @@ export async function activate(context: ExtensionContext) { } } -export function deactivate() { +export function deactivate(): Thenable | undefined { + if (!client) { + return undefined + } return client.stop() } From b0dd1e3fcca801e7bc9142a95937fa1ae0a8719c Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Sat, 30 Mar 2024 18:29:22 +0000 Subject: [PATCH 09/12] Update README to reference shfmt --- README.md | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 94e2126d6..d52e8845f 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # Bash Language Server -Bash language server that brings an IDE-like experience for bash scripts to most editors. This is based on the [Tree Sitter parser][tree-sitter-bash] and supports [explainshell][explainshell] and [shellcheck][shellcheck]. +Bash language server that brings an IDE-like experience for bash scripts to most editors. This is based on the [Tree Sitter parser][tree-sitter-bash] and supports [explainshell][explainshell], [shellcheck][shellcheck] and [shfmt][shfmt]. Documentation around configuration variables can be found in the [config.ts](https://github.com/bash-lsp/bash-language-server/blob/main/server/src/config.ts) file. @@ -15,6 +15,7 @@ Documentation around configuration variables can be found in the [config.ts](htt - Documentation for symbols on hover - Workspace symbols - Rename symbol +- Format document To be implemented: @@ -24,7 +25,11 @@ To be implemented: ### Dependencies -As a dependency, we recommend that you first install shellcheck [shellcheck][shellcheck] to enable linting: https://github.com/koalaman/shellcheck#installing . If shellcheck is installed, bash-language-server will automatically call it to provide linting and code analysis each time the file is updated (with debounce time or 500ms). +As a dependency, we recommend that you first install shellcheck [shellcheck][shellcheck] to enable linting: https://github.com/koalaman/shellcheck#installing . If shellcheck is installed, bash-language-server will automatically call it to provide linting and code analysis each time the file is updated (with debounce time of 500ms). + +If you want your shell scripts to be formatted consistently, you can install [shfmt][shfmt]. If +`shfmt` is installed then your documents will be formatted whenever you take the 'format document' +action. In most editors this can be configured to happen automatically when files are saved. ### Bash language server @@ -197,6 +202,7 @@ Please see [docs/development-guide][dev-guide] for more information. [sublime-text-lsp]: https://packagecontrol.io/packages/LSP-bash [explainshell]: https://explainshell.com/ [shellcheck]: https://www.shellcheck.net/ +[shfmt]: https://github.com/mvdan/sh#shfmt [languageclient-neovim]: https://github.com/autozimu/LanguageClient-neovim [nvim-lspconfig]: https://github.com/neovim/nvim-lspconfig [vim-lsp]: https://github.com/prabirshrestha/vim-lsp From f257ef9958539fe8384864b75612b6679733a7d0 Mon Sep 17 00:00:00 2001 From: Kenneth Skovhus Date: Tue, 30 Apr 2024 12:42:36 +0200 Subject: [PATCH 10/12] Bump server version to 5.3.0 --- server/CHANGELOG.md | 4 ++++ server/package.json | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/server/CHANGELOG.md b/server/CHANGELOG.md index e4ef00f46..cad493c66 100644 --- a/server/CHANGELOG.md +++ b/server/CHANGELOG.md @@ -1,5 +1,9 @@ # Bash Language Server +## 5.3.0 + +- Add support for formatting using shfmt (if installed). https://github.com/bash-lsp/bash-language-server/pull/1136 + ## 5.2.0 - Upgrade tree-sitter-bash from 0.20.7 to 0.22.5 https://github.com/bash-lsp/bash-language-server/pull/1148 diff --git a/server/package.json b/server/package.json index 28a93dbc2..b8238e2ad 100644 --- a/server/package.json +++ b/server/package.json @@ -3,7 +3,7 @@ "description": "A language server for Bash", "author": "Mads Hartmann", "license": "MIT", - "version": "5.2.0", + "version": "5.3.0", "main": "./out/server.js", "typings": "./out/server.d.ts", "bin": { From 8372fe71005950aab05241bcfdd3d3b72a0f1aec Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Tue, 30 Apr 2024 16:33:32 +0100 Subject: [PATCH 11/12] Ensure shfmt is installed in verify workflow shfmt must be installed in order for the tests to pass in the GitHub 'verify' workflow. --- .github/workflows/verify.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/verify.yml b/.github/workflows/verify.yml index e2d68c655..c6a4c3fff 100644 --- a/.github/workflows/verify.yml +++ b/.github/workflows/verify.yml @@ -14,8 +14,8 @@ jobs: steps: - uses: actions/checkout@v4 - - name: Install shellcheck (used for testing) - run: sudo apt-get install -y shellcheck + - name: Install shellcheck and shfmt (used for testing) + run: sudo apt-get install -y shellcheck shfmt - uses: pnpm/action-setup@v2 with: From 9d5d784cb9056be79afb65ee220dc60f351ae684 Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Tue, 30 Apr 2024 22:28:15 +0100 Subject: [PATCH 12/12] Use short options when running shfmt Long options are not supported in shfmt <= 3.5.0. Ubuntu 22.04 packages shfmt 3.4.3. --- server/src/shfmt/index.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/server/src/shfmt/index.ts b/server/src/shfmt/index.ts index e6873a881..d10f5033a 100644 --- a/server/src/shfmt/index.ts +++ b/server/src/shfmt/index.ts @@ -66,11 +66,11 @@ export class Formatter { shfmtConfig?: Record | null, ): Promise { const indentation: number = formatOptions?.insertSpaces ? formatOptions.tabSize : 0 - const args: string[] = [`--indent=${indentation}`] - if (shfmtConfig?.binaryNextLine) args.push('--binary-next-line') - if (shfmtConfig?.caseIndent) args.push('--case-indent') - if (shfmtConfig?.funcNextLine) args.push('--func-next-line') - if (shfmtConfig?.spaceRedirects) args.push('--space-redirects') + const args: string[] = [`-i=${indentation}`] // --indent + if (shfmtConfig?.binaryNextLine) args.push('-bn') // --binary-next-line + if (shfmtConfig?.caseIndent) args.push('-ci') // --case-indent + if (shfmtConfig?.funcNextLine) args.push('-fn') // --func-next-line + if (shfmtConfig?.spaceRedirects) args.push('-sr') // --space-redirects logger.debug(`Shfmt: running "${this.executablePath} ${args.join(' ')}"`)