Feature: Jump to label (#82)

pcj · web-flow · commit d146865e9fc0 · 2021-11-14T08:53:38.000-07:00
* Show buildifier parse error as diagnostic
* Buildifier report error diagnostic on parse error
* Disable bazel doc group hover
diff --git a/package.json b/package.json
@@ -480,6 +480,12 @@
                 "title": "Bazel: Copy label to clipboard",
                 "icon": "$(clippy)"
             },
+            {
+                "category": "Bzl",
+                "command": "bsv.bzl.goToLabel",
+                "title": "Bazel: Goto Label",
+                "icon": "$(go-to-file)"
+            },
             {
                 "category": "Bzl",
                 "command": "bsv.buildozer.wizard",
@@ -578,6 +584,10 @@
                 "command": "bsv.buildozer.wizard",
                 "key": "ctrl+shift+cmd+p"
             },
+            {
+                "command": "bsv.bzl.goToLabel",
+                "key": "cmd+;"
+            },
             {
                 "command": "workbench.view.extension.bazel-explorer",
                 "key": "shift+cmd+t",
diff --git a/src/bazeldoc/feature.ts b/src/bazeldoc/feature.ts
@@ -6,11 +6,17 @@ import { Reconfigurable } from '../reconfigurable';
 
 export const BazelDocFeatureName = 'bsv.bazeldoc';
 
+// this was an early feature; now that we have better LSP completion support,
+// these hovers are just annoying.
+const useBazelDocGroupHover = false;
+
 export class BazelDocFeature extends Reconfigurable<BazelDocConfiguration> {
   constructor() {
     super(BazelDocFeatureName);
 
-    this.add(new BazelDocGroupHover(this.onDidConfigurationChange.event));
+    if (useBazelDocGroupHover) {
+      this.add(new BazelDocGroupHover(this.onDidConfigurationChange.event));
+    }
   }
 
   protected async configure(config: vscode.WorkspaceConfiguration): Promise<BazelDocConfiguration> {
diff --git a/src/bezel/constants.ts b/src/bezel/constants.ts
@@ -47,6 +47,7 @@ export enum CommandName {
   CodesearchIndex = 'bsv.bzl.codesearch.index',
   CodesearchSearch = 'bsv.bzl.codesearch.search',
   CopyLabel = 'bsv.bzl.copyLabel',
+  GoToLabel = 'bsv.bzl.goToLabel',
   CopyToClipboard = 'bsv.bzl.copyToClipboard',
   DebugBuild = 'bsv.bzl.debugBuild',
   AskForDebugTargetLabel = 'bsv.bzl.askForDebugTargetLabel',
diff --git a/src/bezel/feature.ts b/src/bezel/feature.ts
@@ -32,10 +32,10 @@ import { StarlarkDebugger } from './debugger';
 import { RunnableComponent, Status } from './status';
 import { Settings } from './settings';
 import { BuildozerSettings } from '../buildozer/settings';
-import { Buildozer } from '../buildozer/buildozer';
-import { ConfigurationContext, ConfigurationPropertyMap } from '../common';
+import { ConfigurationContext } from '../common';
 import findUp = require('find-up');
 import path = require('path');
+import { Buildozer } from '../buildozer/buildozer';
 
 export const BzlFeatureName = 'bsv.bzl';
 
diff --git a/src/bezel/lsp.ts b/src/bezel/lsp.ts
@@ -7,8 +7,10 @@ import {
   ServerOptions,
   State,
   StateChangeEvent,
+  TextDocumentIdentifier,
   TextDocumentPositionParams,
 } from 'vscode-languageclient/node';
+import { BuiltInCommands } from '../constants';
 import {
   LanguageServerConfiguration,
 } from './configuration';
@@ -32,7 +34,8 @@ export class BzlLanguageClient
     super('LSP', settings);
 
     this.disposables.push(
-      vscode.commands.registerCommand(CommandName.CopyLabel, this.handleCommandCopyLabel, this)
+      vscode.commands.registerCommand(CommandName.CopyLabel, this.handleCommandCopyLabel, this),
+      vscode.commands.registerCommand(CommandName.GoToLabel, this.handleCommandGoToLabel, this),
     );
   }
 
@@ -80,6 +83,47 @@ export class BzlLanguageClient
     }
   }
 
+  private async handleCommandGoToLabel(): Promise<void | undefined> {
+    const label = await vscode.window.showInputBox({
+      prompt: 'Enter bazel label to locate',
+      placeHolder: '//path/to:target',
+    });
+    if (!label) {
+      return;
+    }
+    return this.goToLabel(label);
+  }
+
+  private async goToLabel(label: string): Promise<void | undefined> {
+    const editor = vscode.window.activeTextEditor;
+    if (!editor) {
+      return;
+    }
+    if (!editor.document.uri) {
+      return;
+    }
+    try {
+      const location = await this.getLabelLocation(editor.document.uri, label);
+      if (!location || !location.uri || !location.range) {
+        return;
+      }
+
+      return vscode.commands.executeCommand(
+        BuiltInCommands.Open,
+        vscode.Uri.parse(location.uri).with({
+          // location response is zero-based; convert to 1-base
+          fragment: `${location.range.start.line + 1},${location.range.start.character + 1}`,
+        })
+      );
+    } catch (e) {
+      if (e instanceof Error) {
+        console.debug(e.message);
+      } else {
+        console.debug(JSON.stringify(e));
+      }
+    }
+  }
+
   private async handleCommandCopyLabel(doc: vscode.TextDocument): Promise<void> {
     const editor = vscode.window.activeTextEditor;
     if (!editor) {
@@ -107,6 +151,28 @@ export class BzlLanguageClient
     }
   }
 
+  // getLabelLocation returns the location of the given label.  The URI is used
+  // to resolve non-canonical forms.
+  public async getLabelLocation(
+    uri: vscode.Uri,
+    label: string,
+    cancellation = new vscode.CancellationTokenSource()
+  ): Promise<Location | undefined> {
+    if (!this.languageClient) {
+      return undefined;
+    }
+    const params: BuildFileLabelLocationParams = {
+      textDocument: { uri: uri.toString() },
+      label: label,
+    };
+    const location: Location | undefined = await this.languageClient.sendRequest(
+      'buildFile/labelLocation',
+      params,
+      cancellation.token
+    );
+    return location;
+  }
+
   public async getLabelAtDocumentPosition(
     uri: vscode.Uri,
     position: vscode.Position,
@@ -232,6 +298,11 @@ export interface Invocation {
   createdAt: number;
 }
 
+export interface BuildFileLabelLocationParams {
+  textDocument: TextDocumentIdentifier;
+  label: string;
+}
+
 function createLanguageClient(cfg: LanguageServerConfiguration): LanguageClient {
   let serverOptions: ServerOptions = {
     command: cfg.executable,
diff --git a/src/buildifier/buildifier.ts b/src/buildifier/buildifier.ts
@@ -18,5 +18,5 @@ export class Buildifier extends RunnableComponent<BuildifierConfiguration> {
     await this.settings.get();
   }
 
-  async stopInternal(): Promise<void> {}
+  async stopInternal(): Promise<void> { }
 }
diff --git a/src/buildifier/diagnostics.ts b/src/buildifier/diagnostics.ts
@@ -14,6 +14,7 @@
 
 import * as vscode from 'vscode';
 import { buildifierLint, getBuildifierFileType } from './execute';
+import { IBuildifierWarning } from './result';
 import { BuildifierSettings } from './settings';
 
 /**
@@ -62,6 +63,18 @@ export class BuildifierDiagnosticsManager {
     }
   }
 
+  /**
+   * Sets a single error diagnostic.  This is used when a parsing error occurs
+   * and we want to report the location.
+   *
+   * @param document The text document whose diagnostics should be updated.
+   */
+  public async error(document: vscode.TextDocument, range: vscode.Range, message: string) {
+    const diagnostic = new vscode.Diagnostic(range, message, vscode.DiagnosticSeverity.Error);
+    diagnostic.source = 'buildifier';
+    this.diagnosticsCollection.set(document.uri, [diagnostic]);
+  }
+
   /**
    * Updates the diagnostics collection with lint warnings for the given text
    * document.
@@ -81,32 +94,80 @@ export class BuildifierDiagnosticsManager {
       return;
     }
 
-    const warnings = await buildifierLint(
+    const result = await buildifierLint(
       cfg,
       document.getText(),
       getBuildifierFileType(document.uri.fsPath),
       'warn'
     );
 
-    this.diagnosticsCollection.set(
-      document.uri,
-      warnings.map(warning => {
-        // Buildifier returns 1-based line numbers, but VS Code is 0-based.
-        const range = new vscode.Range(
-          warning.start.line - 1,
-          warning.start.column - 1,
-          warning.end.line - 1,
-          warning.end.column - 1
-        );
-        const diagnostic = new vscode.Diagnostic(
-          range,
-          warning.message,
-          vscode.DiagnosticSeverity.Warning
-        );
-        diagnostic.source = 'buildifier';
-        diagnostic.code = warning.category;
-        return diagnostic;
-      })
-    );
+    if (!result.stderr) {
+      this.diagnosticsCollection.set(
+        document.uri,
+        result.file.warnings.map(warning => {
+          // Buildifier returns 1-based line numbers, but VS Code is 0-based.
+          const range = new vscode.Range(
+            warning.start.line - 1,
+            warning.start.column - 1,
+            warning.end.line - 1,
+            warning.end.column - 1
+          );
+          const diagnostic = new vscode.Diagnostic(
+            range,
+            warning.message,
+            vscode.DiagnosticSeverity.Warning
+          );
+          diagnostic.source = 'buildifier';
+          diagnostic.code = warning.category;
+          return diagnostic;
+        })
+      );
+    } else {
+      const warnings: IBuildifierWarning[] = [];
+      const syntaxError = parseStderr(result.stderr);
+      if (syntaxError) {
+        warnings.push(syntaxError);
+      }
+      this.diagnosticsCollection.set(
+        document.uri,
+        warnings.map(warning => {
+          // Buildifier returns 1-based line numbers, but VS Code is 0-based.
+          const range = new vscode.Range(
+            warning.start.line - 1,
+            warning.start.column - 1,
+            warning.end.line - 1,
+            warning.end.column - 1
+          );
+          const diagnostic = new vscode.Diagnostic(
+            range,
+            warning.message,
+            vscode.DiagnosticSeverity.Error
+          );
+          diagnostic.source = 'buildifier';
+          diagnostic.code = warning.category;
+          return diagnostic;
+        })
+      );
+
+    }
   }
 }
+
+function parseStderr(input: string): IBuildifierWarning | undefined {
+  if (!input.startsWith('<stdin>')) {
+    return undefined;
+  }
+  const parts = input.slice('<stdin>:'.length).split(':');
+  const line = parseInt(parts[0]);
+  const column = parseInt(parts[1]);
+  const rest = parts.slice(2).join(':');
+  const pos = { line, column };
+
+  return {
+    start: pos,
+    end: pos,
+    category: 'invalid-input',
+    actionable: false,
+    message: rest,
+  }
+}
diff --git a/src/buildifier/execute.ts b/src/buildifier/execute.ts
@@ -14,7 +14,7 @@
 
 import * as child_process from 'child_process';
 import * as path from 'path';
-import { IBuildifierResult, IBuildifierWarning } from './result';
+import { IBuildifierResult, IBuildifierStdinResult } from './result';
 import { BuildifierConfiguration } from './configuration';
 
 /** Whether to warn about lint findings or fix them. */
@@ -81,14 +81,14 @@ export async function buildifierLint(
   fileContent: string,
   type: BuildifierFileType,
   lintMode: 'warn'
-): Promise<IBuildifierWarning[]>;
+): Promise<IBuildifierStdinResult>;
 
 export async function buildifierLint(
   cfg: BuildifierConfiguration,
   fileContent: string,
   type: BuildifierFileType,
   lintMode: BuildifierLintMode
-): Promise<string | IBuildifierWarning[]> {
+): Promise<string | IBuildifierStdinResult> {
   const args = ['--format=json', '--mode=check', `--type=${type}`, `--lint=${lintMode}`];
   const outputs = await executeBuildifier(cfg, fileContent, args, true);
   switch (lintMode) {
@@ -98,10 +98,23 @@ export async function buildifierLint(
       const result = JSON.parse(outputs.stdout) as IBuildifierResult;
       for (const file of result.files) {
         if (file.filename === '<stdin>') {
-          return file.warnings;
+          return {
+            success: result.success,
+            file: file,
+            stderr: outputs.stderr
+          }
         }
       }
-      return [];
+      // should not occur
+      return {
+        success: false,
+        file: {
+          filename: '<stdin>',
+          formatted: false,
+          valid: false,
+          warnings: [],
+        }
+      };
   }
 }
 
diff --git a/src/buildifier/formatter.ts b/src/buildifier/formatter.ts
@@ -70,7 +70,9 @@ export class BuildifierFormatter implements vscode.DocumentFormattingEditProvide
         ),
       ];
     } catch (err) {
-      vscode.window.showErrorMessage(`buildifier format error: ${err}`);
+      // linter should report the syntactic error, ignore this for the user, but
+      // report debugging info
+      console.warn('buildifier error:', err);
     }
 
     return [];
diff --git a/src/buildifier/result.ts b/src/buildifier/result.ts
@@ -23,6 +23,20 @@ export interface IBuildifierResult {
   files: IBuildifierFile[];
 }
 
+/** The result of a single file result from stdin (typical case) */
+export interface IBuildifierStdinResult {
+  /**
+   * Indicates whether or not the check succeeded without finding any problems.
+   */
+  success: boolean;
+
+  /** Information about each file that was checked. */
+  file: IBuildifierFile;
+
+  /** Stderr, if the input was not valid */
+  stderr?: string;
+}
+
 /** Information about a file that was checked by buildifier. */
 export interface IBuildifierFile {
   /** The path of the file that was checked. */
diff --git a/src/test/integration/feature.buildifier.test.ts b/src/test/integration/feature.buildifier.test.ts

Original file line number	Diff line number	Diff line change
`@@ -18,5 +18,5 @@ export class Buildifier extends RunnableComponent<BuildifierConfiguration> {`
`18`	`18`	`await this.settings.get();`
`19`	`19`	`}`
`20`	`20`
`21`		`- async stopInternal(): Promise<void> {}`
	`21`	`+ async stopInternal(): Promise<void> { }`
`22`	`22`	`}`