[PATCH] D66211: [clangd][vscode] Surface the error when applying tweaks fails

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368851: [clangd][vscode] Surface the error when applying 
tweaks fails (authored by hokein, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66211?vs=215107&id=215109#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66211/new/

https://reviews.llvm.org/D66211

Files:
  clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts


Index: clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
===
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
@@ -47,6 +47,23 @@
   dispose() { this.statusBarItem.dispose(); }
 }
 
+class ClangdLanguageClient extends vscodelc.LanguageClient {
+  // Override the default implementation for failed requests. The default
+  // behavior is just to log failures in the output panel, however output panel
+  // is designed for extension debugging purpose, normal users will not open 
it,
+  // thus when the failure occurs, normal users doesn't know that.
+  //
+  // For user-interactive operations (e.g. applyFixIt, applyTweaks), we will
+  // prompt up the failure to users.
+  logFailedRequest(rpcReply: vscodelc.RPCMessageType, error: any) {
+if (error instanceof vscodelc.ResponseError &&
+rpcReply.method === "workspace/executeCommand")
+  vscode.window.showErrorMessage(error.message);
+// Call default implementation.
+super.logFailedRequest(rpcReply, error);
+  }
+}
+
 /**
  *  this method is called when your extension is activate
  *  your extension is activated the very first time the command is executed
@@ -89,8 +106,8 @@
 revealOutputChannelOn: vscodelc.RevealOutputChannelOn.Never
 };
 
-  const clangdClient = new vscodelc.LanguageClient(
-  'Clang Language Server', serverOptions, clientOptions);
+  const clangdClient = new ClangdLanguageClient('Clang Language Server',
+serverOptions, clientOptions);
   console.log('Clang Language Server is now active!');
   context.subscriptions.push(clangdClient.start());
   context.subscriptions.push(vscode.commands.registerCommand(


Index: clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
===
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
@@ -47,6 +47,23 @@
   dispose() { this.statusBarItem.dispose(); }
 }
 
+class ClangdLanguageClient extends vscodelc.LanguageClient {
+  // Override the default implementation for failed requests. The default
+  // behavior is just to log failures in the output panel, however output panel
+  // is designed for extension debugging purpose, normal users will not open it,
+  // thus when the failure occurs, normal users doesn't know that.
+  //
+  // For user-interactive operations (e.g. applyFixIt, applyTweaks), we will
+  // prompt up the failure to users.
+  logFailedRequest(rpcReply: vscodelc.RPCMessageType, error: any) {
+if (error instanceof vscodelc.ResponseError &&
+rpcReply.method === "workspace/executeCommand")
+  vscode.window.showErrorMessage(error.message);
+// Call default implementation.
+super.logFailedRequest(rpcReply, error);
+  }
+}
+
 /**
  *  this method is called when your extension is activate
  *  your extension is activated the very first time the command is executed
@@ -89,8 +106,8 @@
 revealOutputChannelOn: vscodelc.RevealOutputChannelOn.Never
 };
 
-  const clangdClient = new vscodelc.LanguageClient(
-  'Clang Language Server', serverOptions, clientOptions);
+  const clangdClient = new ClangdLanguageClient('Clang Language Server',
+serverOptions, clientOptions);
   console.log('Clang Language Server is now active!');
   context.subscriptions.push(clangdClient.start());
   context.subscriptions.push(vscode.commands.registerCommand(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66211: [clangd][vscode] Surface the error when applying tweaks fails

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts:109
+// Keep the default behavior.
+if (error instanceof vscodelc.ResponseError &&
+error.code === vscodelc.ErrorCodes.RequestCancelled)

ilya-biryukov wrote:
> Is this how default implementation behaves? Is there a way to call into it 
> instead of re-implementing?
> If so, it would be nice to do so. If not, I'm also ok if we keep it.
manage to figure out a way to do it. We have to define a subclass of 
LanguageClient, and override logFailedRequest. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66211/new/

https://reviews.llvm.org/D66211



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66211: [clangd][vscode] Surface the error when applying tweaks fails

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 215107.
hokein marked an inline comment as done.
hokein added a comment.

avoid re-implementing of the method.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66211/new/

https://reviews.llvm.org/D66211

Files:
  clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts


Index: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
@@ -47,6 +47,20 @@
   dispose() { this.statusBarItem.dispose(); }
 }
 
+class ClangdLanguageClient extends vscodelc.LanguageClient {
+  // Override the default implementation for failed requests. The default
+  // behavior is just to log failures in the output panel, however output panel
+  // is designed for extension debugging purpose, normal users will not open 
it,
+  // thus when the failure occurs, normal users doesn't know that.
+  logFailedRequest(rpcReply: vscodelc.RPCMessageType, error: any) {
+if (error instanceof vscodelc.ResponseError &&
+rpcReply.method === "workspace/executeCommand")
+  vscode.window.showErrorMessage(error.message);
+// Call default implementation.
+super.logFailedRequest(rpcReply, error);
+  }
+}
+
 /**
  *  this method is called when your extension is activate
  *  your extension is activated the very first time the command is executed
@@ -89,8 +103,8 @@
 revealOutputChannelOn: vscodelc.RevealOutputChannelOn.Never
 };
 
-  const clangdClient = new vscodelc.LanguageClient(
-  'Clang Language Server', serverOptions, clientOptions);
+  const clangdClient = new ClangdLanguageClient('Clang Language Server',
+serverOptions, clientOptions);
   console.log('Clang Language Server is now active!');
   context.subscriptions.push(clangdClient.start());
   context.subscriptions.push(vscode.commands.registerCommand(


Index: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
@@ -47,6 +47,20 @@
   dispose() { this.statusBarItem.dispose(); }
 }
 
+class ClangdLanguageClient extends vscodelc.LanguageClient {
+  // Override the default implementation for failed requests. The default
+  // behavior is just to log failures in the output panel, however output panel
+  // is designed for extension debugging purpose, normal users will not open it,
+  // thus when the failure occurs, normal users doesn't know that.
+  logFailedRequest(rpcReply: vscodelc.RPCMessageType, error: any) {
+if (error instanceof vscodelc.ResponseError &&
+rpcReply.method === "workspace/executeCommand")
+  vscode.window.showErrorMessage(error.message);
+// Call default implementation.
+super.logFailedRequest(rpcReply, error);
+  }
+}
+
 /**
  *  this method is called when your extension is activate
  *  your extension is activated the very first time the command is executed
@@ -89,8 +103,8 @@
 revealOutputChannelOn: vscodelc.RevealOutputChannelOn.Never
 };
 
-  const clangdClient = new vscodelc.LanguageClient(
-  'Clang Language Server', serverOptions, clientOptions);
+  const clangdClient = new ClangdLanguageClient('Clang Language Server',
+serverOptions, clientOptions);
   console.log('Clang Language Server is now active!');
   context.subscriptions.push(clangdClient.start());
   context.subscriptions.push(vscode.commands.registerCommand(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66211: [clangd][vscode] Surface the error when applying tweaks fails

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

Thanks for providing examples of error messages, also agree they look fine.
If we find bad error messages later, we could revisit the presentation strategy.
LGTM




Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts:109
+// Keep the default behavior.
+if (error instanceof vscodelc.ResponseError &&
+error.code === vscodelc.ErrorCodes.RequestCancelled)

Is this how default implementation behaves? Is there a way to call into it 
instead of re-implementing?
If so, it would be nice to do so. If not, I'm also ok if we keep it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66211/new/

https://reviews.llvm.org/D66211



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66211: [clangd][vscode] Surface the error when applying tweaks fails

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D66211#1629112 , @ilya-biryukov 
wrote:

> How do they look in practice? In particular, should we add more context 
> information (either in clangd or in the VSCode extension) to those errors, 
> now that we know they are shown to the users?
>  Something like `Failed to apply a fix: input range is invalid` vs `input 
> range is invalid`.


I don't have a very strong preference on adding more context, the current 
messages seem good enough to me, something like:

- "edits were not applied: "
- "Could not expand type of lambda expression:"
- "Could not obtain range of the 'else' branch. Macros?"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66211/new/

https://reviews.llvm.org/D66211



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66211: [clangd][vscode] Surface the error when applying tweaks fails

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Surfacing errors to the users in those cases is definitely something we need to 
do, thanks!
How do they look in practice? In particular, should we add more context 
information (either in clangd or in the VSCode extension) to those errors, now 
that we know they are shown to the users?
Something like `Failed to apply a fix: input range is invalid` vs `input range 
is invalid`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66211/new/

https://reviews.llvm.org/D66211



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66211: [clangd][vscode] Surface the error when applying tweaks fails

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: ilya-biryukov.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay.
Herald added a project: clang.

The current behavior for a failed request is just to log it in the
output panel. When applyTweak fails for whatever reason, users usually don't get
informed (unless they open the output panel and dig the log).

this patch is to surface these errors by prompting up a message diag.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66211

Files:
  clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts


Index: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
@@ -91,6 +91,26 @@
 
   const clangdClient = new vscodelc.LanguageClient(
   'Clang Language Server', serverOptions, clientOptions);
+
+  // We override the default implementation for failed requests. The default
+  // behavior is just to log failures in the output panel, however output panel
+  // is designed for extension debugging purpose, normal users will not open 
it,
+  // thus when the failure occurs, normal users doesn't know that.
+  //
+  // For user-interactive operations (e.g. applyFixIt, applyTweaks), we will 
prompt
+  // up the failure to users.
+  clangdClient.logFailedRequest =
+  (rpcReply: vscodelc.RPCMessageType, error: any) => {
+if (error instanceof vscodelc.ResponseError &&
+rpcReply.method === "workspace/executeCommand")
+  vscode.window.showErrorMessage(error.message);
+
+// Keep the default behavior.
+if (error instanceof vscodelc.ResponseError &&
+error.code === vscodelc.ErrorCodes.RequestCancelled)
+  return;
+clangdClient.error(`Request ${rpcReply.method} failed.`, error);
+  };
   console.log('Clang Language Server is now active!');
   context.subscriptions.push(clangdClient.start());
   context.subscriptions.push(vscode.commands.registerCommand(


Index: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
@@ -91,6 +91,26 @@
 
   const clangdClient = new vscodelc.LanguageClient(
   'Clang Language Server', serverOptions, clientOptions);
+
+  // We override the default implementation for failed requests. The default
+  // behavior is just to log failures in the output panel, however output panel
+  // is designed for extension debugging purpose, normal users will not open it,
+  // thus when the failure occurs, normal users doesn't know that.
+  //
+  // For user-interactive operations (e.g. applyFixIt, applyTweaks), we will prompt
+  // up the failure to users.
+  clangdClient.logFailedRequest =
+  (rpcReply: vscodelc.RPCMessageType, error: any) => {
+if (error instanceof vscodelc.ResponseError &&
+rpcReply.method === "workspace/executeCommand")
+  vscode.window.showErrorMessage(error.message);
+
+// Keep the default behavior.
+if (error instanceof vscodelc.ResponseError &&
+error.code === vscodelc.ErrorCodes.RequestCancelled)
+  return;
+clangdClient.error(`Request ${rpcReply.method} failed.`, error);
+  };
   console.log('Clang Language Server is now active!');
   context.subscriptions.push(clangdClient.start());
   context.subscriptions.push(vscode.commands.registerCommand(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits