Merge pull request #694 from fortran-lang/gnikit/issue693

bug(lsp): fixes relative path resolution in fortls

Author: gnikit

CHANGELOG.md

@@ -69,6 +69,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Fixed
 
+- Fixed bugs in relative path resolution for `fortls`
+  ([#693](https://github.com/fortran-lang/vscode-fortran-support/issues/693))
 - Fixed issues with linter unittests running asynchronously
   ([#623](https://github.com/fortran-lang/vscode-fortran-support/pull/623))
 - Fixed `npm run watch-dev` not syncing changes to spawned Extension Dev Host

package.json

@@ -722,7 +722,7 @@
     "webpack": "webpack --mode production",
     "pretest": "npm run compile-dev && tsc -p tsconfig.test.json",
     "test": "npm run test:unit && npm run test:integration && npm run test:ui",
-    "test:ui": "extest setup-and-run -i out/test/ui/*.js -m test/ui/.mocharc.js -s .vscode-test -e .vscode-test/extensions",
+    "test:ui": "extest setup-and-run -i out/test/ui/*.js -m test/ui/.mocharc.js -s .vscode-test/ui -e .vscode-test/ui/extensions",
     "test:unit": "node ./out/test/unitTest/runTest.js",
     "test:integration": "node ./out/test/integration/runTest.js",
     "test:grammar-free": "vscode-tmgrammar-snap -s source.fortran.free -g ./syntaxes/fortran_free-form.tmLanguage.json \"./test/fortran/syntax/**/*{.f90,F90}\"",

src/lsp/client.ts

@@ -1,5 +1,6 @@
 'use strict';
 
+import * as os from 'os';
 import * as path from 'path';
 import * as vscode from 'vscode';
 import { spawnSync } from 'child_process';
@@ -13,7 +14,6 @@ import {
   getOuterMostWorkspaceFolder,
   pipInstall,
   resolveVariables,
-  pathRelToAbs,
 } from '../lib/tools';
 import { Logger } from '../services/logging';
 import { RestartLS } from '../features/commands';
@@ -84,9 +84,7 @@ export class FortlsClient {
     if (!isFortran(document)) return;
 
     const args: string[] = await this.fortlsArguments();
-    const fortlsPath = workspace.getConfiguration(EXTENSION_ID).get<string>('fortls.path');
-    let executablePath = resolveVariables(fortlsPath);
-    if (fortlsPath !== 'fortls') executablePath = pathRelToAbs(fortlsPath, document.uri);
+    const executablePath: string = await this.fortlsPath(document);
 
     // Detect language server version and verify selected options
     this.version = this.getLSVersion(executablePath, args);
@@ -308,14 +306,7 @@ export class FortlsClient {
    */
   private async fortlsDownload(): Promise<boolean> {
     const config = workspace.getConfiguration(EXTENSION_ID);
-    let ls = resolveVariables(config.get<string>('fortls.path'));
-    // The path can be resolved as a relative path if it's part of a workspace
-    // AND it does not have the default value `fortls` or is an absolute path
-    if (workspace.workspaceFolders == undefined && ls !== 'fortls' && !path.isAbsolute(ls)) {
-      const root = workspace.workspaceFolders[0];
-      this.logger.debug(`[lsp.client] Assuming relative fortls path is to ${root.uri.fsPath}`);
-      ls = pathRelToAbs(ls, root.uri);
-    }
+    const ls = await this.fortlsPath();
 
     // Check for version, if this fails fortls provided is invalid
     const results = spawnSync(ls, ['--version']);
@@ -349,6 +340,50 @@ export class FortlsClient {
     });
   }
 
+  /**
+   * Try and find the path to the `fortls` executable.
+   * It will first try and fetch the top-most workspaceFolder from `document`.
+   * If that fails because the document is standalone and does not belong in a
+   * workspace it will assume that relative paths are wrt `os.homedir()`.
+   *
+   * If the `document` argument is missing, then it will try and find the
+   * first workspaceFolder and use that as the root. If that fails it will
+   * revert back to `os.homedir()`.
+   *
+   * @param document Optional textdocument
+   * @returns a promise with the path to the fortls executable
+   */
+  private async fortlsPath(document?: TextDocument): Promise<string> {
+    // Get the workspace folder that contains the document, this can be undefined
+    // which means that the document is standalone and not part of any workspace.
+    let folder: vscode.WorkspaceFolder | undefined;
+    if (document) {
+      folder = workspace.getWorkspaceFolder(document.uri);
+    }
+    // If the document argument is missing, such as in the case of the Client's
+    // activation, then try and fetch the first workspace folder to use as a root.
+    else {
+      folder = workspace.workspaceFolders[0] ? workspace.workspaceFolders[0] : undefined;
+    }
+
+    // Get the outer most workspace folder to resolve relative paths, but if
+    // the folder is undefined then use the home directory of the OS
+    const root = folder ? getOuterMostWorkspaceFolder(folder).uri : vscode.Uri.parse(os.homedir());
+
+    const config = workspace.getConfiguration(EXTENSION_ID);
+    let executablePath = resolveVariables(config.get<string>('fortls.path'));
+
+    // The path can be resolved as a relative path if:
+    // 1. it does not have the default value `fortls` AND
+    // 2. is not an absolute path
+    if (executablePath !== 'fortls' && !path.isAbsolute(executablePath)) {
+      this.logger.debug(`[lsp.client] Assuming relative fortls path is to ${root.fsPath}`);
+      executablePath = path.join(root.fsPath, executablePath);
+    }
+
+    return executablePath;
+  }
+
   /**
    * Restart the language server
    */

test/fortran/.vscode/settings.json

@@ -7,6 +7,7 @@
     "DEBUG": "1",
     "VAL": ""
   },
+  "fortran.fortls.path": "fortls",
   "fortran.fortls.disableAutoupdate": true,
   "fortran.fortls.preprocessor.definitions": {
     "HAVE_ZOLTAN": "",

test/integration/lsp-client.test.ts

@@ -46,4 +46,32 @@ suite('Language Server integration tests', () => {
     const res = server['client']?.initializeResult;
     strictEqual(JSON.stringify(ref), JSON.stringify(res));
   });
+
+  test('Restart the Language Server', async () => {
+    await server['restartLS']();
+    await delay(3000); // wait for server to initialize
+
+    const ref = {
+      capabilities: {
+        completionProvider: {
+          resolveProvider: false,
+          triggerCharacters: ['%'],
+        },
+        definitionProvider: true,
+        documentSymbolProvider: true,
+        referencesProvider: true,
+        hoverProvider: true,
+        implementationProvider: true,
+        renameProvider: true,
+        workspaceSymbolProvider: true,
+        textDocumentSync: 2,
+        signatureHelpProvider: {
+          triggerCharacters: ['(', ','],
+        },
+        codeActionProvider: true,
+      },
+    };
+    const res = server['client']?.initializeResult;
+    strictEqual(JSON.stringify(ref), JSON.stringify(res));
+  });
 });

test/unitTest/client.test.ts

@@ -0,0 +1,43 @@
+import * as vscode from 'vscode';
+import * as path from 'path';
+import { strictEqual } from 'assert';
+import { FortlsClient } from '../../src/lsp/client';
+import { Logger, LogLevel } from '../../src/services/logging';
+import { EXTENSION_ID } from '../../src/lib/tools';
+
+const logger = new Logger(
+  vscode.window.createOutputChannel('Modern Fortran', 'log'),
+  LogLevel.DEBUG
+);
+
+suite('Language Server integration tests', () => {
+  const server = new FortlsClient(logger);
+  const fileUri = vscode.Uri.file(path.resolve(__dirname, '../../../test/fortran/sample.f90'));
+  const config = vscode.workspace.getConfiguration(EXTENSION_ID);
+  const oldVal = config.get<string>('fortls.path');
+  let doc: vscode.TextDocument;
+
+  suiteSetup(async () => {
+    await config.update('fortls.path', './venv/bin/fortls', false);
+    doc = await vscode.workspace.openTextDocument(fileUri);
+  });
+
+  test('Local path resolution (Document URI)', async () => {
+    const ls = await server['fortlsPath'](doc);
+    const root = path.resolve(__dirname, '../../../test/fortran/');
+    const ref = path.resolve(root, './venv/bin/fortls');
+    console.log(`Reference: ${ref}`);
+    strictEqual(ls, ref);
+  });
+
+  test('Local path resolution (Document missing workspaceFolders[0])', async () => {
+    const ls = await server['fortlsPath']();
+    const root = path.resolve(__dirname, '../../../test/fortran/');
+    const ref = path.resolve(root, './venv/bin/fortls');
+    strictEqual(ls, ref);
+  });
+
+  suiteTeardown(async () => {
+    await config.update('fortls.path', oldVal, false);
+  });
+});