Merge pull request #1230 from github/aeisenberg/astviewer-uri

Fix invalid file comparison for changing ast viewer location

extensions/ql-vscode/CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## [UNRELEASED]
 
+- Fix a bug where the AST viewer was not synchronizing its selected node when the editor selection changes. [#1230](https://github.com/github/vscode-codeql/pull/1230)
+
 ## 1.6.1 - 17 March 2022
 
 No user facing changes.

extensions/ql-vscode/src/astViewer.ts

@@ -10,7 +10,8 @@ import {
   TextEditorSelectionChangeEvent,
   TextEditorSelectionChangeKind,
   Location,
-  Range
+  Range,
+  Uri
 } from 'vscode';
 import * as path from 'path';
 
@@ -104,7 +105,7 @@ class AstViewerDataProvider extends DisposableObject implements TreeDataProvider
 export class AstViewer extends DisposableObject {
   private treeView: TreeView<AstItem>;
   private treeDataProvider: AstViewerDataProvider;
-  private currentFile: string | undefined;
+  private currentFileUri: Uri | undefined;
 
   constructor() {
     super();
@@ -125,12 +126,12 @@ export class AstViewer extends DisposableObject {
     this.push(window.onDidChangeTextEditorSelection(this.updateTreeSelection, this));
   }
 
-  updateRoots(roots: AstItem[], db: DatabaseItem, fileName: string) {
+  updateRoots(roots: AstItem[], db: DatabaseItem, fileUri: Uri) {
     this.treeDataProvider.roots = roots;
     this.treeDataProvider.db = db;
     this.treeDataProvider.refresh();
-    this.treeView.message = `AST for ${path.basename(fileName)}`;
-    this.currentFile = fileName;
+    this.treeView.message = `AST for ${path.basename(fileUri.fsPath)}`;
+    this.currentFileUri = fileUri;
     // Handle error on reveal. This could happen if
     // the tree view is disposed during the reveal.
     this.treeView.reveal(roots[0], { focus: false })?.then(
@@ -174,7 +175,7 @@ export class AstViewer extends DisposableObject {
 
     if (
       this.treeView.visible &&
-      e.textEditor.document.uri.fsPath === this.currentFile &&
+      e.textEditor.document.uri.fsPath === this.currentFileUri?.fsPath &&
       e.selections.length === 1
     ) {
       const selection = e.selections[0];
@@ -199,6 +200,6 @@ export class AstViewer extends DisposableObject {
     this.treeDataProvider.db = undefined;
     this.treeDataProvider.refresh();
     this.treeView.message = undefined;
-    this.currentFile = undefined;
+    this.currentFileUri = undefined;
   }
 }

extensions/ql-vscode/src/contextual/astBuilder.ts

@@ -4,6 +4,7 @@ import { DecodedBqrsChunk, BqrsId, EntityValue } from '../pure/bqrs-cli-types';
 import { DatabaseItem } from '../databases';
 import { ChildAstItem, AstItem } from '../astViewer';
 import fileRangeFromURI from './fileRangeFromURI';
+import { Uri } from 'vscode';
 
 /**
  * A class that wraps a tree of QL results from a query that
@@ -17,7 +18,7 @@ export default class AstBuilder {
     queryResults: QueryWithResults,
     private cli: CodeQLCliServer,
     public db: DatabaseItem,
-    public fileName: string
+    public fileName: Uri
   ) {
     this.bqrsPath = queryResults.query.resultsPaths.resultsPath;
   }

extensions/ql-vscode/src/contextual/templateProvider.ts

@@ -10,7 +10,6 @@ import {
   TextDocument,
   Uri
 } from 'vscode';
-import * as path from 'path';
 
 import { decodeSourceArchiveUri, encodeArchiveBasePath, zipArchiveScheme } from '../archive-filesystem-provider';
 import { CodeQLCliServer } from '../cli';
@@ -160,7 +159,7 @@ export class TemplatePrintAstProvider {
     return new AstBuilder(
       query, this.cli,
       this.dbm.findDatabaseItem(dbUri)!,
-      path.basename(fileUri.fsPath),
+      fileUri,
     );
   }
 

extensions/ql-vscode/src/vscode-tests/no-workspace/astViewer.test.ts

@@ -5,7 +5,7 @@ import * as sinon from 'sinon';
 import * as yaml from 'js-yaml';
 
 import { AstViewer, AstItem } from '../../astViewer';
-import { commands, Range } from 'vscode';
+import { commands, Range, Uri } from 'vscode';
 import { DatabaseItem } from '../../databases';
 import { testDisposeHandler } from '../test-dispose-handler';
 
@@ -40,7 +40,7 @@ describe('AstViewer', () => {
   it('should update the viewer roots', () => {
     const item = {} as DatabaseItem;
     viewer = new AstViewer();
-    viewer.updateRoots(astRoots, item, 'def/abc');
+    viewer.updateRoots(astRoots, item, Uri.file('def/abc'));
 
     expect((viewer as any).treeDataProvider.roots).to.eq(astRoots);
     expect((viewer as any).treeDataProvider.db).to.eq(item);
@@ -59,25 +59,31 @@ describe('AstViewer', () => {
     doSelectionTest(expr, expr.fileLocation?.range);
   });
 
-  it('should select nothing', () => {
+  it('should select nothing because of no overlap in range', () => {
     doSelectionTest(undefined, new Range(2, 3, 4, 5));
   });
 
+  it('should select nothing because of different file', () => {
+    doSelectionTest(undefined, astRoots[0].fileLocation?.range, Uri.file('def'));
+  });
+
+  const defaultUri = Uri.file('def/abc');
+
   function doSelectionTest(
     expectedSelection: any,
     selectionRange: Range | undefined,
-    fsPath = 'def/abc',
+    fileUri = defaultUri
   ) {
     const item = {} as DatabaseItem;
     viewer = new AstViewer();
-    viewer.updateRoots(astRoots, item, fsPath);
+    viewer.updateRoots(astRoots, item, defaultUri);
     const spy = sandbox.spy();
     (viewer as any).treeView.reveal = spy;
     Object.defineProperty((viewer as any).treeView, 'visible', {
       value: true
     });
 
-    const mockEvent = createMockEvent(selectionRange, fsPath);
+    const mockEvent = createMockEvent(selectionRange, fileUri);
     (viewer as any).updateTreeSelection(mockEvent);
     if (expectedSelection) {
       expect(spy).to.have.been.calledWith(expectedSelection);
@@ -88,7 +94,7 @@ describe('AstViewer', () => {
 
   function createMockEvent(
     selectionRange: Range | undefined,
-    fsPath: string,
+    uri: Uri,
   ) {
     return {
       selections: [{
@@ -98,7 +104,7 @@ describe('AstViewer', () => {
       textEditor: {
         document: {
           uri: {
-            fsPath
+            fsPath: uri.fsPath
           }
         }
       }

extensions/ql-vscode/src/vscode-tests/no-workspace/contextual/astBuilder.test.ts

@@ -7,6 +7,7 @@ import AstBuilder from '../../../contextual/astBuilder';
 import { QueryWithResults } from '../../../run-queries';
 import { CodeQLCliServer } from '../../../cli';
 import { DatabaseItem } from '../../../databases';
+import { Uri } from 'vscode';
 
 chai.use(chaiAsPromised);
 const expect = chai.expect;
@@ -145,7 +146,7 @@ describe('AstBuilder', () => {
           resultsPath: '/a/b/c'
         }
       }
-    } as QueryWithResults, mockCli, {} as DatabaseItem, '');
+    } as QueryWithResults, mockCli, {} as DatabaseItem, Uri.file(''));
   }
 
   function mockDecode(resultSet: 'nodes' | 'edges' | 'graphProperties') {