Merge pull request #127 from krassowski/rename_syntax_error_ux_workar…

…ound

Implement a UX workaround for rope/pyls rename issue

packages/jupyterlab-lsp/src/adapters/codemirror/features/diagnostics.ts

@@ -1,14 +1,27 @@
 import * as CodeMirror from 'codemirror';
 import * as lsProtocol from 'vscode-languageserver-protocol';
 import { PositionConverter } from '../../../converter';
-import { IVirtualPosition } from '../../../positioning';
+import { IEditorPosition, IVirtualPosition } from '../../../positioning';
 import { diagnosticSeverityNames } from '../../../lsp';
 import { DefaultMap } from '../../../utils';
 import { CodeMirrorLSPFeature } from '../feature';
 
 // TODO: settings
 const default_severity = 2;
 
+/**
+ * Diagnostic which is localized at a specific editor (cell) within a notebook
+ * (if used in the context of a FileEditor, then there is just a single editor)
+ */
+export interface IEditorDiagnostic {
+  diagnostic: lsProtocol.Diagnostic;
+  editor: CodeMirror.Editor;
+  range: {
+    start: IEditorPosition;
+    end: IEditorPosition;
+  };
+}
+
 export class Diagnostics extends CodeMirrorLSPFeature {
   register(): void {
     this.connection_handlers.set(
@@ -21,6 +34,14 @@ export class Diagnostics extends CodeMirrorLSPFeature {
 
   private unique_editor_ids: DefaultMap<CodeMirror.Editor, number>;
   private marked_diagnostics: Map<string, CodeMirror.TextMarker> = new Map();
+  /**
+   * Allows access to the most recent diagnostics in context of the editor.
+   *
+   * One can use VirtualEditorForNotebook.find_cell_by_editor() to find
+   * the corresponding cell in notebook.
+   * Can be used to implement a Panel showing diagnostics list.
+   */
+  public diagnostics_db: IEditorDiagnostic[];
 
   protected collapse_overlapping_diagnostics(
     diagnostics: lsProtocol.Diagnostic[]
@@ -74,6 +95,7 @@ export class Diagnostics extends CodeMirrorLSPFeature {
   public handleDiagnostic(response: lsProtocol.PublishDiagnosticsParams) {
     /* TODO: gutters */
     try {
+      let diagnostics_db: IEditorDiagnostic[] = [];
       // Note: no deep equal for Sets or Maps in JS
       const markers_to_retain: Set<string> = new Set();
 
@@ -146,6 +168,10 @@ export class Diagnostics extends CodeMirrorLSPFeature {
 
           let start_in_editor = document.transform_virtual_to_editor(start);
           let end_in_editor = document.transform_virtual_to_editor(end);
+          let range_in_editor = {
+            start: start_in_editor,
+            end: end_in_editor
+          };
           // what a pity there is no hash in the standard library...
           // we could use this: https://stackoverflow.com/a/7616484 though it may not be worth it:
           //   the stringified diagnostic objects are only about 100-200 JS characters anyway,
@@ -166,12 +192,17 @@ export class Diagnostics extends CodeMirrorLSPFeature {
             // after the (inserted/removed) line - but such markers should not be invalidated,
             // i.e. the invalidation should be performed in the cell space, not in the notebook coordinate space,
             // thus we transform the coordinates and keep the cell id in the hash
-            range: {
-              start: start_in_editor,
-              end: end_in_editor
-            },
+            range: range_in_editor,
             editor: this.unique_editor_ids.get(cm_editor)
           });
+          for (let diagnostic of diagnostics) {
+            diagnostics_db.push({
+              diagnostic,
+              editor: cm_editor,
+              range: range_in_editor
+            });
+          }
+
           markers_to_retain.add(diagnostic_hash);
 
           if (!this.marked_diagnostics.has(diagnostic_hash)) {
@@ -201,6 +232,8 @@ export class Diagnostics extends CodeMirrorLSPFeature {
 
       // remove the markers which were not included in the new message
       this.remove_unused_diagnostic_markers(markers_to_retain);
+
+      this.diagnostics_db = diagnostics_db;
     } catch (e) {
       console.warn(e);
     }

packages/jupyterlab-lsp/src/adapters/codemirror/features/rename.ts

@@ -5,16 +5,36 @@ import {
   IFeatureCommand
 } from '../feature';
 import { InputDialog } from '@jupyterlab/apputils';
+import { Diagnostics } from './diagnostics';
+import { VirtualEditor } from '../../../virtual/editor';
+import { VirtualEditorForNotebook } from '../../../virtual/editors/notebook';
 
 export class Rename extends CodeMirrorLSPFeature {
   static commands: Array<IFeatureCommand> = [
     {
       id: 'rename-symbol',
-      execute: ({ connection, virtual_position, document, features }) => {
+      execute: ({
+        editor,
+        connection,
+        virtual_position,
+        document,
+        features
+      }) => {
         let old_value = document.getTokenAt(virtual_position).string;
         let handle_failure = (error: any) => {
-          let feature = features.get('Rename') as Rename;
-          feature.status_message.set(`Rename failed: ${error}`, 5 * 1000);
+          let rename_feature = features.get('Rename') as Rename;
+          let diagnostics_feature = features.get('Diagnostics') as Diagnostics;
+
+          let status = ux_workaround_for_rope_limitation(
+            error,
+            diagnostics_feature,
+            editor
+          );
+          if (!status) {
+            status = `Rename failed: ${error}`;
+          }
+
+          rename_feature.status_message.set(status, 7.5 * 1000);
         };
 
         InputDialog.getText({
@@ -64,3 +84,58 @@ export class Rename extends CodeMirrorLSPFeature {
       .catch(console.warn);
   }
 }
+
+/**
+ * In #115 an issue with rename for Python (when using pyls) was identified:
+ * rename was failing with an obscure message when the source code could
+ * not be parsed correctly by rope (due to a user's syntax error).
+ *
+ * This function detects such a condition using diagnostics feature
+ * and provides a nice error message to the user.
+ */
+function ux_workaround_for_rope_limitation(
+  error: any,
+  diagnostics_feature: Diagnostics,
+  editor: VirtualEditor
+): string {
+  let has_index_error = false;
+  try {
+    has_index_error = error.message.includes('IndexError');
+  } catch (e) {
+    return null;
+  }
+  if (!has_index_error) {
+    return null;
+  }
+  let dire_python_errors = (diagnostics_feature.diagnostics_db || []).filter(
+    diagnostic =>
+      diagnostic.diagnostic.message.includes('invalid syntax') ||
+      diagnostic.diagnostic.message.includes('SyntaxError') ||
+      diagnostic.diagnostic.message.includes('IndentationError')
+  );
+
+  if (dire_python_errors.length === 0) {
+    return null;
+  }
+
+  let dire_errors = [
+    ...new Set(
+      dire_python_errors.map(diagnostic => {
+        let message = diagnostic.diagnostic.message;
+        let start = diagnostic.range.start;
+        if (editor.has_cells) {
+          let notebook_editor = editor as VirtualEditorForNotebook;
+          let { cell_id } = notebook_editor.find_cell_by_editor(
+            diagnostic.editor
+          );
+          let cell_nr = cell_id + 1;
+          // TODO: should we show "code cell" numbers, or just cell number?
+          return `${message} in cell ${cell_nr} at line ${start.line}`;
+        } else {
+          return `${message} at line ${start.line}`;
+        }
+      })
+    )
+  ].join(', ');
+  return `Syntax error(s) prevent rename: ${dire_errors}`;
+}

packages/jupyterlab-lsp/src/adapters/jupyterlab/jl_adapter.ts

@@ -390,12 +390,17 @@ export abstract class JupyterLabWidgetAdapter
   get_context_from_context_menu(): ICommandContext {
     let root_position = this.get_position_from_context_menu();
     let document = this.virtual_editor.document_at_root_position(root_position);
-    let connection = this.connection_manager.connections.get(document.id_path);
     let virtual_position = this.virtual_editor.root_position_to_virtual_position(
       root_position
     );
-    let features = this.get_features(document);
-    return { document, connection, virtual_position, root_position, features };
+    return {
+      document,
+      connection: this.connection_manager.connections.get(document.id_path),
+      virtual_position,
+      root_position,
+      features: this.get_features(document),
+      editor: this.virtual_editor
+    };
   }
 
   public create_tooltip(

packages/jupyterlab-lsp/src/command_manager.ts

@@ -13,6 +13,7 @@ import { INotebookTracker } from '@jupyterlab/notebook';
 import { VirtualDocument } from './virtual/document';
 import { LSPConnection } from './connection';
 import { IRootPosition, IVirtualPosition } from './positioning';
+import { VirtualEditor } from './virtual/editor';
 
 export const file_editor_adapters: Map<string, FileEditorAdapter> = new Map();
 export const notebook_adapters: Map<string, NotebookAdapter> = new Map();
@@ -158,4 +159,5 @@ export interface ICommandContext {
   virtual_position: IVirtualPosition;
   root_position: IRootPosition;
   features: Map<string, ILSPFeature>;
+  editor: VirtualEditor;
 }

packages/jupyterlab-lsp/src/virtual/editors/notebook.ts

@@ -38,10 +38,6 @@ class DocDispatcher implements CodeMirror.Doc {
       );
   }
 
-  // getValue(seperator?: string): string {
-  //  return this.virtual_editor.getValue();
-  // }
-
   getCursor(start?: string): CodeMirror.Position {
     let cell = this.virtual_editor.notebook.activeCell;
     let active_editor = cell.editor as CodeMirrorEditor;
@@ -360,4 +356,24 @@ export class VirtualEditorForNotebook extends VirtualEditor {
       });
     }
   }
+
+  /**
+   * Find a cell in notebook which uses given CodeMirror editor.
+   * This function is O(n) - when looking up many cells
+   * using a hashmap based approach may be more efficient.
+   * @param cm_editor
+   */
+  find_cell_by_editor(cm_editor: CodeMirror.Editor) {
+    let cells = this.notebook.widgets;
+    for (let i = 0; i < cells.length; i++) {
+      let cell = cells[i];
+      let cell_editor = (cell.editor as CodeMirrorEditor).editor;
+      if (cell_editor === cm_editor) {
+        return {
+          cell_id: i,
+          cell: cell
+        };
+      }
+    }
+  }
 }