From f08a99bb35410003b5ed89f02753dff8b06439e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Maci=C4=85=C5=BCek?= Date: Wed, 4 Aug 2021 23:50:01 +0200 Subject: [PATCH 1/4] Fix special characters in workspace and file paths (#15) --- src/services/ansibleConfig.ts | 2 +- src/services/ansibleLint.ts | 12 +++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/services/ansibleConfig.ts b/src/services/ansibleConfig.ts index 495c4f4f..b188bee2 100644 --- a/src/services/ansibleConfig.ts +++ b/src/services/ansibleConfig.ts @@ -35,7 +35,7 @@ export class AnsibleConfig { const ansibleConfigResult = child_process.execSync(ansibleConfigCommand, { encoding: 'utf-8', - cwd: new URL(this.context.workspaceFolder.uri).pathname, + cwd: decodeURI(new URL(this.context.workspaceFolder.uri).pathname), env: ansibleConfigEnv, }); diff --git a/src/services/ansibleLint.ts b/src/services/ansibleLint.ts index af489c6f..1cdf1a6f 100644 --- a/src/services/ansibleLint.ts +++ b/src/services/ansibleLint.ts @@ -50,7 +50,7 @@ export class AnsibleLint { public async doValidate( textDocument: TextDocument ): Promise> { - const docPath = new URL(textDocument.uri).pathname; + const docPath = decodeURI(new URL(textDocument.uri).pathname); let diagnostics: Map = new Map(); let progressTracker; if (this.useProgressTracker) { @@ -61,7 +61,9 @@ export class AnsibleLint { textDocument.uri ); - const workingDirectory = new URL(this.context.workspaceFolder.uri).pathname; + const workingDirectory = decodeURI( + new URL(this.context.workspaceFolder.uri).pathname + ); try { const settings = await this.context.documentSettings.get( @@ -79,7 +81,7 @@ export class AnsibleLint { const [command, env] = withInterpreter( settings.ansibleLint.path, - `${settings.ansibleLint.arguments} --offline --nocolor -f codeclimate ${docPath}`, + `${settings.ansibleLint.arguments} --offline --nocolor -f codeclimate "${docPath}"`, settings.python.interpreterPath, settings.python.activationScript ); @@ -192,8 +194,8 @@ export class AnsibleLint { } } } - - const locationUri = `file://${workingDirectory}/${item.location.path}`; + const path = `${workingDirectory}/${item.location.path}`; + const locationUri = `file://${encodeURI(path)}`; let fileDiagnostics = diagnostics.get(locationUri); if (!fileDiagnostics) { From 2a9e66f7bc46b0d1bbdd1f95d7ae4b51480b2931 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Maci=C4=85=C5=BCek?= Date: Wed, 4 Aug 2021 23:57:00 +0200 Subject: [PATCH 2/4] Advise yamllint installation and use (#16) --- README.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index ba5e7b56..c0b163f5 100644 --- a/README.md +++ b/README.md @@ -55,6 +55,10 @@ rules/tags added to `warn_list` (see [Ansible Lint Documentation](https://ansible-lint.readthedocs.io/en/latest/configuring.html)) are shown as warnings instead. +> If you also install `yamllint`, `ansible-lint` will detect it and incorporate +> into the linting process. Any findings reported by `yamllint` will be exposed +> in VSCode as errors/warnings. + ### Smart autocompletion ![Autocompletion](images/smart-completions.gif) @@ -92,7 +96,8 @@ holding `ctrl`/`cmd`. ## Requirements - [Ansible 2.9+](https://docs.ansible.com/ansible/latest/index.html) - [Ansible Lint](https://ansible-lint.readthedocs.io/en/latest/) (required, - unless you disable linter support; install without `yamllint`) + unless you disable linter support) +- [yamllint](https://yamllint.readthedocs.io/en/stable/) (optional) For Windows users, this extension works perfectly well with extensions such as `Remote - WSL` and `Remote - Containers`. From 7d044ba44d8dca7b6f12b4bb7670ab9e60a076f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Maci=C4=85=C5=BCek?= Date: Sun, 8 Aug 2021 23:11:00 +0200 Subject: [PATCH 3/4] Fix handling of files with CRLF line endings (#20) --- src/providers/completionProvider.ts | 10 +++--- src/providers/definitionProvider.ts | 14 +++++--- src/providers/hoverProvider.ts | 13 ++++---- src/providers/semanticTokenProvider.ts | 10 +++--- src/providers/validationProvider.ts | 12 +++++-- src/utils/yaml.ts | 45 ++++++++++++++++++++++---- 6 files changed, 76 insertions(+), 28 deletions(-) diff --git a/src/providers/completionProvider.ts b/src/providers/completionProvider.ts index 15cd0147..9cf0bef7 100644 --- a/src/providers/completionProvider.ts +++ b/src/providers/completionProvider.ts @@ -6,7 +6,6 @@ import { TextEdit, } from 'vscode-languageserver'; import { Position, TextDocument } from 'vscode-languageserver-textdocument'; -import { parseAllDocuments } from 'yaml'; import { Node, Pair, Scalar, YAMLMap } from 'yaml/types'; import { IOption } from '../interfaces/module'; import { WorkspaceFolderContext } from '../services/workspaceManager'; @@ -24,11 +23,13 @@ import { findProvidedModule, getDeclaredCollections, getPathAt, + getOrigRange, getYamlMapKeys, isBlockParam, isPlayParam, isRoleParam, isTaskParam, + parseAllDocuments, } from '../utils/yaml'; const priorityMap = { @@ -293,9 +294,10 @@ function getKeywordCompletion( * the node has range information and is a string scalar. */ function getNodeRange(node: Node, document: TextDocument): Range | undefined { - if (node.range && node instanceof Scalar && typeof node.value === 'string') { - const start = node.range[0]; - let end = node.range[1]; + const range = getOrigRange(node); + if (range && node instanceof Scalar && typeof node.value === 'string') { + const start = range[0]; + let end = range[1]; // compensate for `_:` if (node.value.includes('_:')) { end -= 2; diff --git a/src/providers/definitionProvider.ts b/src/providers/definitionProvider.ts index cae248b3..3161fcc6 100644 --- a/src/providers/definitionProvider.ts +++ b/src/providers/definitionProvider.ts @@ -1,10 +1,15 @@ import { DefinitionLink, Range } from 'vscode-languageserver'; import { Position, TextDocument } from 'vscode-languageserver-textdocument'; -import { parseAllDocuments } from 'yaml'; import { Scalar } from 'yaml/types'; import { DocsLibrary } from '../services/docsLibrary'; import { toLspRange } from '../utils/misc'; -import { AncestryBuilder, getPathAt, isTaskParam } from '../utils/yaml'; +import { + AncestryBuilder, + getOrigRange, + getPathAt, + isTaskParam, + parseAllDocuments, +} from '../utils/yaml'; export async function getDefinition( document: TextDocument, @@ -26,11 +31,12 @@ export async function getDefinition( document.uri ); if (module) { + const range = getOrigRange(node); return [ { targetUri: module.source, - originSelectionRange: node.range - ? toLspRange(node.range, document) + originSelectionRange: range + ? toLspRange(range, document) : undefined, targetRange: Range.create( module.sourceLineRange[0], diff --git a/src/providers/hoverProvider.ts b/src/providers/hoverProvider.ts index 71dfec7d..f5914c64 100644 --- a/src/providers/hoverProvider.ts +++ b/src/providers/hoverProvider.ts @@ -1,6 +1,5 @@ import { Hover, MarkupContent, MarkupKind } from 'vscode-languageserver'; import { Position, TextDocument } from 'vscode-languageserver-textdocument'; -import { parseAllDocuments } from 'yaml'; import { Scalar, YAMLMap } from 'yaml/types'; import { DocsLibrary } from '../services/docsLibrary'; import { @@ -19,11 +18,13 @@ import { toLspRange } from '../utils/misc'; import { AncestryBuilder, findProvidedModule, + getOrigRange, getPathAt, isBlockParam, isPlayParam, isRoleParam, isTaskParam, + parseAllDocuments, } from '../utils/yaml'; export async function doHover( @@ -60,13 +61,14 @@ export async function doHover( path, document.uri ); + const range = getOrigRange(node); if (module && module.documentation) { return { contents: formatModule( module.documentation, docsLibrary.getModuleRoute(hitFqcn || node.value) ), - range: node.range ? toLspRange(node.range, document) : undefined, + range: range ? toLspRange(range, document) : undefined, }; } else if (hitFqcn) { // check for tombstones @@ -74,9 +76,7 @@ export async function doHover( if (route) { return { contents: formatTombstone(route), - range: node.range - ? toLspRange(node.range, document) - : undefined, + range: range ? toLspRange(range, document) : undefined, }; } } @@ -136,9 +136,10 @@ function getKeywordHover( } : keywordDocumentation; if (markupDoc) { + const range = getOrigRange(node); return { contents: markupDoc, - range: node.range ? toLspRange(node.range, document) : undefined, + range: range ? toLspRange(range, document) : undefined, }; } else return null; } diff --git a/src/providers/semanticTokenProvider.ts b/src/providers/semanticTokenProvider.ts index e97df193..56689945 100644 --- a/src/providers/semanticTokenProvider.ts +++ b/src/providers/semanticTokenProvider.ts @@ -5,7 +5,6 @@ import { SemanticTokenTypes, } from 'vscode-languageserver'; import { TextDocument } from 'vscode-languageserver-textdocument'; -import { parseAllDocuments } from 'yaml'; import { Node, Pair, Scalar, YAMLMap, YAMLSeq } from 'yaml/types'; import { IModuleMetadata } from '../interfaces/module'; import { DocsLibrary } from '../services/docsLibrary'; @@ -17,10 +16,12 @@ import { } from '../utils/ansible'; import { findProvidedModule, + getOrigRange, isBlockParam, isPlayParam, isRoleParam, isTaskParam, + parseAllDocuments, } from '../utils/yaml'; export const tokenTypes = [ @@ -233,9 +234,10 @@ function markNode( builder: SemanticTokensBuilder, document: TextDocument ) { - if (node.range) { - const startPosition = document.positionAt(node.range[0]); - const length = node.range[1] - node.range[0]; + const range = getOrigRange(node); + if (range) { + const startPosition = document.positionAt(range[0]); + const length = range[1] - range[0]; builder.push( startPosition.line, startPosition.character, diff --git a/src/providers/validationProvider.ts b/src/providers/validationProvider.ts index eeecb362..2f44a2f1 100644 --- a/src/providers/validationProvider.ts +++ b/src/providers/validationProvider.ts @@ -8,9 +8,9 @@ import { Range, } from 'vscode-languageserver'; import { TextDocument } from 'vscode-languageserver-textdocument'; -import { parseAllDocuments } from 'yaml'; import { ValidationManager } from '../services/validationManager'; import { WorkspaceFolderContext } from '../services/workspaceManager'; +import { parseAllDocuments } from '../utils/yaml'; /** * Validates the given document. @@ -63,8 +63,14 @@ function getYamlValidation(textDocument: TextDocument): Diagnostic[] { const errorRange = error.range || error.source?.range; let range; if (errorRange) { - const start = textDocument.positionAt(errorRange.start); - const end = textDocument.positionAt(errorRange.end); + const start = textDocument.positionAt( + errorRange.origStart !== undefined + ? errorRange.origStart + : errorRange.start + ); + const end = textDocument.positionAt( + errorRange.origEnd !== undefined ? errorRange.origEnd : errorRange.end + ); range = Range.create(start, end); let severity; diff --git a/src/utils/yaml.ts b/src/utils/yaml.ts index 14dcd021..a68c7662 100644 --- a/src/utils/yaml.ts +++ b/src/utils/yaml.ts @@ -1,6 +1,6 @@ import * as _ from 'lodash'; import { Position, TextDocument } from 'vscode-languageserver-textdocument'; -import { Document } from 'yaml'; +import { Document, Options, parseCST } from 'yaml'; import { Node, Pair, Scalar, YAMLMap, YAMLSeq } from 'yaml/types'; import { IModuleMetadata } from '../interfaces/module'; import { DocsLibrary } from '../services/docsLibrary'; @@ -131,7 +131,7 @@ export class AncestryBuilder { export function getPathAt( document: TextDocument, position: Position, - docs: Document.Parsed[], + docs: Document[], inclusive = false ): Node[] | null { const offset = document.offsetAt(position); @@ -147,10 +147,11 @@ export function contains( offset: number, inclusive: boolean ): boolean { + const range = getOrigRange(node); return !!( - node?.range && - node.range[0] <= offset && - (node.range[1] > offset || (inclusive && node.range[1] >= offset)) + range && + range[0] <= offset && + (range[1] > offset || (inclusive && range[1] >= offset)) ); } @@ -180,8 +181,8 @@ export function getPathAtOffset( } pair = _.find(currentNode.items, (p) => { const inBetweenNode = new Node(); - const start = (p.key as Node)?.range?.[1]; - const end = (p.value as Node)?.range?.[0]; + const start = getOrigRange(p.key as Node)?.[1]; + const end = getOrigRange(p.value as Node)?.[0]; if (start && end) { inBetweenNode.range = [start, end - 1]; return contains(inBetweenNode, offset, inclusive); @@ -390,3 +391,33 @@ export function getYamlMapKeys(mapNode: YAMLMap): Array { } }); } + +export function getOrigRange( + node: Node | null | undefined +): [number, number] | null | undefined { + if (node?.cstNode?.range) { + const range = node.cstNode.range; + return [ + range.origStart !== undefined ? range.origStart : range.start, + range.origEnd !== undefined ? range.origEnd : range.end, + ]; + } else { + return node?.range; + } +} + +/** Parsing with the YAML library tailored to the needs of this extension */ +export function parseAllDocuments(str: string, options?: Options): Document[] { + const cst = parseCST(str); + cst.setOrigRanges(); + + const parsedDocuments: Document[] = []; + for (const cstDoc of cst) { + const parsedDocument = new Document( + Object.assign({ keepCstNodes: true }, options) + ); + parsedDocument.parse(cstDoc); + parsedDocuments.push(parsedDocument); + } + return parsedDocuments; +} From 8596747d78d9143a667325e5fde41740e61801f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Maci=C4=85=C5=BCek?= Date: Tue, 10 Aug 2021 22:06:06 +0200 Subject: [PATCH 4/4] Mimic linter's config file search algorithm (#22) Since ansible-lint is executed from the root directory of the workspace the algorithm that it uses to find configuration for a particular file is not involved. To compensate, the extension now mimics that behavior, searching the directory structure, going up from the investigated file. At the same time, it is still possible to force a particular config file through arguments provided in the extension settings. The configuration file that is actually used is now correctly identified for the purpose of marking configured finding groups as warnings. --- src/services/ansibleLint.ts | 116 +++++++++++++++++++++--------------- 1 file changed, 69 insertions(+), 47 deletions(-) diff --git a/src/services/ansibleLint.ts b/src/services/ansibleLint.ts index 1cdf1a6f..0f7308c9 100644 --- a/src/services/ansibleLint.ts +++ b/src/services/ansibleLint.ts @@ -1,6 +1,7 @@ import * as child_process from 'child_process'; import { ExecException } from 'child_process'; import { promises as fs } from 'fs'; +import * as path from 'path'; import { URL } from 'url'; import { promisify } from 'util'; import { @@ -50,27 +51,47 @@ export class AnsibleLint { public async doValidate( textDocument: TextDocument ): Promise> { - const docPath = decodeURI(new URL(textDocument.uri).pathname); let diagnostics: Map = new Map(); - let progressTracker; - if (this.useProgressTracker) { - progressTracker = await this.connection.window.createWorkDoneProgress(); - } - - const ansibleLintConfigPromise = this.getAnsibleLintConfig( - textDocument.uri - ); const workingDirectory = decodeURI( new URL(this.context.workspaceFolder.uri).pathname ); - try { - const settings = await this.context.documentSettings.get( - textDocument.uri + const settings = await this.context.documentSettings.get(textDocument.uri); + + if (settings.ansibleLint.enabled) { + let linterArguments = settings.ansibleLint.arguments; + + // Determine linter config file + let ansibleLintConfigPath = linterArguments.match( + /(?:^|\s)-c\s*(?[\s'"])(?.+?)(?:\k|$)/ + )?.groups?.conf; + if (!ansibleLintConfigPath) { + // Config file not provided in arguments -> search for one mimicking the + // way ansible-lint looks for it, going up the directory structure + const ansibleLintConfigFile = await this.findAnsibleLintConfigFile( + textDocument.uri + ); + if (ansibleLintConfigFile) { + ansibleLintConfigPath = decodeURI( + new URL(ansibleLintConfigFile).pathname + ); + linterArguments = `${linterArguments} -c "${ansibleLintConfigPath}"`; + } + } + linterArguments = `${linterArguments} --offline --nocolor -f codeclimate`; + + const docPath = decodeURI(new URL(textDocument.uri).pathname); + let progressTracker; + if (this.useProgressTracker) { + progressTracker = await this.connection.window.createWorkDoneProgress(); + } + const ansibleLintConfigPromise = this.getAnsibleLintConfig( + workingDirectory, + ansibleLintConfigPath ); - if (settings.ansibleLint.enabled) { + try { if (progressTracker) { progressTracker.begin( 'ansible-lint', @@ -81,7 +102,7 @@ export class AnsibleLint { const [command, env] = withInterpreter( settings.ansibleLint.path, - `${settings.ansibleLint.arguments} --offline --nocolor -f codeclimate "${docPath}"`, + `${linterArguments} --offline --nocolor -f codeclimate "${docPath}"`, settings.python.interpreterPath, settings.python.activationScript ); @@ -100,36 +121,36 @@ export class AnsibleLint { if (result.stderr) { this.connection.console.info(`[ansible-lint] ${result.stderr}`); } - } - } catch (error) { - if (error instanceof Error) { - const execError = error as ExecException & { - // according to the docs, these are always available - stdout: string; - stderr: string; - }; - if (execError.code === 2) { - diagnostics = this.processReport( - execError.stdout, - await ansibleLintConfigPromise, - workingDirectory - ); - } else { - this.connection.window.showErrorMessage(execError.message); - } + } catch (error) { + if (error instanceof Error) { + const execError = error as ExecException & { + // according to the docs, these are always available + stdout: string; + stderr: string; + }; + if (execError.code === 2) { + diagnostics = this.processReport( + execError.stdout, + await ansibleLintConfigPromise, + workingDirectory + ); + } else { + this.connection.window.showErrorMessage(execError.message); + } - if (execError.stderr) { - this.connection.console.info(`[ansible-lint] ${execError.stderr}`); + if (execError.stderr) { + this.connection.console.info(`[ansible-lint] ${execError.stderr}`); + } + } else { + this.connection.console.error( + `Exception in AnsibleLint service: ${JSON.stringify(error)}` + ); } - } else { - this.connection.console.error( - `Exception in AnsibleLint service: ${JSON.stringify(error)}` - ); } - } - if (progressTracker) { - progressTracker.done(); + if (progressTracker) { + progressTracker.done(); + } } return diagnostics; } @@ -244,14 +265,15 @@ export class AnsibleLint { } private async getAnsibleLintConfig( - uri: string + workingDirectory: string, + configPath: string | undefined ): Promise { - const configPath = await this.getAnsibleLintConfigPath(uri); if (configPath) { - let config = this.configCache.get(configPath); + const absConfigPath = path.resolve(workingDirectory, configPath); + let config = this.configCache.get(absConfigPath); if (!config) { - config = await this.readAnsibleLintConfig(configPath); - this.configCache.set(configPath, config); + config = await this.readAnsibleLintConfig(absConfigPath); + this.configCache.set(absConfigPath, config); } return config; } @@ -264,7 +286,7 @@ export class AnsibleLint { warnList: new Set(), }; try { - const configContents = await fs.readFile(new URL(configPath), { + const configContents = await fs.readFile(configPath, { encoding: 'utf8', }); parseAllDocuments(configContents).forEach((configDoc) => { @@ -286,7 +308,7 @@ export class AnsibleLint { return config; } - private async getAnsibleLintConfigPath( + private async findAnsibleLintConfigFile( uri: string ): Promise { // find configuration path