Merge pull request #611 from fortran-lang/feature/linter-cache

Feature/linter cache

Author: gnikit

Diff for: CHANGELOG.md

@@ -16,6 +16,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Changed
 
+- Changed how caching is performed in the linter; generalised code and improved
+  performance of the cache
+  ([#611](https://github.com/fortran-lang/vscode-fortran-support/pull/611))
 - Changed packaging download to use `vscode.Tasks`
   ([#608](https://github.com/fortran-lang/vscode-fortran-support/issues/608))
 - Renamed the default repository branch from `master` to `main`

Diff for: src/features/linter-provider.ts

@@ -6,19 +6,18 @@ import which from 'which';
 
 import * as vscode from 'vscode';
 import { Logger } from '../services/logging';
-import { FortranDocumentSelector, resolveVariables } from '../lib/tools';
-import * as fg from 'fast-glob';
-import { glob } from 'glob';
+import { EXTENSION_ID, FortranDocumentSelector, resolveVariables } from '../lib/tools';
 import { arraysEqual } from '../lib/helper';
 import { RescanLint } from './commands';
+import { GlobPaths } from '../lib/glob-paths';
 
 export class FortranLintingProvider {
   constructor(private logger: Logger = new Logger()) {}
 
   private diagnosticCollection: vscode.DiagnosticCollection;
   private compiler: string;
   private compilerPath: string;
-  private cache = { includePaths: [''], globIncPaths: [''] };
+  private pathCache = new Map<string, GlobPaths>();
 
   public provideCodeActions(
     document: vscode.TextDocument,
@@ -128,7 +127,10 @@ export class FortranLintingProvider {
       ...this.getLinterExtraArgs(this.compiler),
       ...this.getModOutputDir(this.compiler),
     ];
-    const includePaths = this.getIncludePaths();
+    const opt = 'linter.includePaths';
+    const includePaths = this.getGlobPathsFromSettings(opt);
+    this.logger.debug(`[lint] glob paths:`, this.pathCache.get(opt).globs);
+    this.logger.debug(`[lint] resolved paths:`, this.pathCache.get(opt).paths);
 
     const extensionIndex = textDocument.fileName.lastIndexOf('.');
     const fileNameWithoutExtension = textDocument.fileName.substring(0, extensionIndex);
@@ -179,61 +181,38 @@ export class FortranLintingProvider {
    * for the `linter.includePaths` option. The results are stored in a cache
    * to improve performance
    *
+   * @param opt String representing a VS Code setting e.g. `linter.includePaths`
+   *
    * @returns String Array of directories
    */
-  private getIncludePaths(): string[] {
-    const config = vscode.workspace.getConfiguration('fortran');
-    let includePaths: string[] = config.get('linter.includePaths', []);
-
-    // Check if we can use the cached results for the include directories no
-    // need to evaluate the glob patterns everytime we call the linter
-    // TODO: register command that forces re-linting
-    // Not sure what the best approach for this one is?
-    // Should I add a watcher to all the files under globIncPaths?
-    // Should I add a watcher on the files under the workspace?
-    if (arraysEqual(includePaths, this.cache['includePaths'])) {
-      return this.cache['globIncPaths'];
-    }
-
-    // Update our cache input
-    this.cache['includePaths'] = includePaths;
-    // Output the original include paths
-    if (includePaths.length > 0) this.logger.debug(`[lint] include:`, includePaths);
-    // Resolve internal variables and expand glob patterns
-    const resIncludePaths = includePaths.map(e => resolveVariables(e));
-    // fast-glob cannot work with Windows paths
-    includePaths = includePaths.map(e => e.replace('/\\/g', '/'));
-    // This needs to be after the resolvevariables since {} are used in globs
-    try {
-      const globIncPaths: string[] = fg.sync(resIncludePaths, {
-        onlyDirectories: true,
-        suppressErrors: false,
-      });
-      // Update values in cache
-      this.cache['globIncPaths'] = globIncPaths;
-      return globIncPaths;
-      // Try to recover from fast-glob failing due to EACCES using slower more
-      // robust glob.
-    } catch (eacces) {
-      this.logger.warn(`[lint] You lack read permissions for an include directory
-          or more likely a glob match from the input 'includePaths' list. This can happen when
-          using overly broad root level glob patters e.g. /usr/lib/** .
-          No reason to worry. I will attempt to recover. 
-          You should consider adjusting your 'includePaths' if linting performance is slow.`);
-      this.logger.warn(`[lint] ${eacces.message}`);
+  private getGlobPathsFromSettings(opt: string): string[] {
+    const config = vscode.workspace.getConfiguration(EXTENSION_ID);
+    const globPaths: string[] = config.get(opt);
+    // Initialise cache key and value if vscode option is not present
+    if (!this.pathCache.has(opt)) {
+      this.logger.debug(`[lint] Initialising cache for ${opt}`);
       try {
-        const globIncPaths: string[] = [];
-        for (const i of resIncludePaths) {
-          // use '/' to match only directories and not files
-          globIncPaths.push(...glob.sync(i + '/', { strict: false }));
-        }
-        this.cache['globIncPaths'] = globIncPaths;
-        return globIncPaths;
-        // if we failed again then our includes are somehow wrong. Abort
+        this.pathCache.set(opt, new GlobPaths(globPaths));
       } catch (error) {
-        this.logger.error(`[lint] Include path glob resolution failed to recover: ${error}`);
+        const msg = `[lint] Error initialising cache for ${opt}`;
+        this.logger.error(msg, error);
+        vscode.window.showErrorMessage(`${msg}: ${error}`);
       }
     }
+    // Check if cache is valid, and if so return cached value
+    if (arraysEqual(globPaths, this.pathCache.get(opt).globs)) {
+      return this.pathCache.get(opt).paths;
+    }
+    // Update cache and return new values
+    try {
+      this.pathCache.get(opt).update(globPaths);
+    } catch (error) {
+      const msg = `[lint] Error initialising cache for ${opt}`;
+      this.logger.error(msg, error);
+      vscode.window.showErrorMessage(`${msg}: ${error}`);
+    }
+    this.logger.debug(`[lint] ${opt} changed, updating cache`);
+    return this.pathCache.get(opt).paths;
   }
 
   /**
@@ -543,10 +522,12 @@ export class FortranLintingProvider {
    * Regenerate the cache for the include files paths of the linter
    */
   private rescanLinter() {
+    const opt = 'linter.includePaths';
     this.logger.debug(`[lint] Resetting linter include paths cache`);
-    this.logger.debug(`[lint] Current linter include paths cache:`, this.cache['includePaths']);
-    this.cache['includePaths'] = [];
-    this.getIncludePaths();
-    this.logger.debug(`[lint] New linter include paths cache:`, this.cache['includePaths']);
+    this.logger.debug(`[lint] Current linter include paths cache:`, this.pathCache.get(opt).globs);
+    this.pathCache.set(opt, new GlobPaths());
+    this.getGlobPathsFromSettings(opt);
+    this.logger.debug(`[lint] glob paths:`, this.pathCache.get(opt).globs);
+    this.logger.debug(`[lint] resolved paths:`, this.pathCache.get(opt).paths);
   }
 }

Diff for: src/lib/glob-paths.ts

@@ -0,0 +1,63 @@
+'use strict';
+
+import * as fg from 'fast-glob';
+import { glob } from 'glob';
+
+import { resolveVariables } from './tools';
+
+/**
+ * A class meant to resolve glob patterns to absolute paths.
+ * Glob resolution can be expensive, so this class can be used for caching
+ */
+export class GlobPaths {
+  public globs: string[] = [];
+  public paths: string[] = [];
+  constructor(globs: string[] = []) {
+    this.update(globs);
+  }
+
+  public update(globs: string[]): void {
+    this.globs = globs;
+    this.paths = this.globResolution(globs);
+  }
+
+  public add(globs: string[]): void {
+    this.globs = this.globs.concat(globs);
+    this.paths = this.paths.concat(this.globResolution(globs));
+  }
+
+  /**
+   *
+   * @param globPaths A list of glob paths to resolve
+   * @returns A list of resolved paths
+   */
+  private globResolution(globPaths: string[]): string[] {
+    if (globPaths.length === 0) return [];
+    // Resolve internal variables and expand glob patterns
+    const globPathsVars = globPaths.map(e => resolveVariables(e));
+    // fast-glob cannot work with Windows paths
+    globPaths = globPaths.map(e => e.replace('/\\/g', '/'));
+    // This needs to be after the resolvevariables since {} are used in globs
+    try {
+      const globIncPaths: string[] = fg.sync(globPathsVars, {
+        onlyDirectories: true,
+        suppressErrors: false,
+      });
+      return globIncPaths;
+      // Try to recover from fast-glob failing due to EACCES using slower more
+      // robust glob.
+    } catch (eacces) {
+      try {
+        const globIncPaths: string[] = [];
+        for (const i of globPathsVars) {
+          // use '/' to match only directories and not files
+          globIncPaths.push(...glob.sync(i + '/', { strict: false }));
+        }
+        return globIncPaths;
+        // if we failed again then our globs are somehow wrong. Abort
+      } catch (error) {
+        throw new TypeError(`Invalid glob syntax, error: ${error}`);
+      }
+    }
+  }
+}

Diff for: test/fortran/.vscode/settings.json

@@ -4,6 +4,7 @@
     "HAVE_ZOLTAN": "",
     "PetscInt": "integer(kind=selected_int_kind(5))"
   },
+  "fortran.linter.includePaths": ["${workspaceFolder}/lint/**"],
   "fortran.fortls.preprocessor.directories": ["include", "val"],
   "fortran.fortls.preprocessor.suffixes": [".fypp", ".f90"],
   "fortran.fortls.suffixes": [".FFF", ".F90-2018"],

Diff for: test/fortran/lint/lvl1-a/.gitkeep

@@ -1,8 +1,70 @@
 import * as path from 'path';
 import { strictEqual, deepStrictEqual } from 'assert';
-import { Diagnostic, DiagnosticSeverity, Range, Position, window, workspace, Uri } from 'vscode';
+import {
+  Diagnostic,
+  DiagnosticSeverity,
+  Range,
+  Position,
+  window,
+  workspace,
+  Uri,
+  TextDocument,
+} from 'vscode';
+import * as fg from 'fast-glob';
+import * as cp from 'child_process';
+
 import { FortranLintingProvider } from '../src/features/linter-provider';
 import { delay } from '../src/lib/helper';
+import { EXTENSION_ID } from '../src/lib/tools';
+
+suite('Linter integration', () => {
+  let doc: TextDocument;
+  const linter = new FortranLintingProvider();
+  const fileUri = Uri.file(path.resolve(__dirname, '../../test/fortran/lint/test1.f90'));
+  const root = path.resolve(__dirname, '../../test/fortran/');
+  const config = workspace.getConfiguration(EXTENSION_ID);
+  const oldVals = config.get<string[]>('linter.includePaths');
+
+  suiteSetup(async function (): Promise<void> {
+    doc = await workspace.openTextDocument(fileUri);
+    await window.showTextDocument(doc);
+  });
+
+  test('Include path globs & internal variable resolution', async () => {
+    const paths = linter['getGlobPathsFromSettings']('linter.includePaths');
+    const refs: string[] = fg.sync(path.dirname(fileUri.path) + '/**', { onlyDirectories: true });
+    deepStrictEqual(paths, refs);
+  });
+
+  test('Path cache contains expected values', async () => {
+    let refs: string[] = ['${workspaceFolder}/lint/**'];
+    deepStrictEqual(linter['pathCache'].get('linter.includePaths')?.globs, refs);
+    refs = fg.sync(path.join(root, 'lint') + '/**', { onlyDirectories: true });
+    deepStrictEqual(linter['pathCache'].get('linter.includePaths')?.paths, refs);
+  });
+
+  test('Update paths using cache', async () => {
+    const refs: string[] = fg.sync([path.join(root, 'lint') + '/**', path.join(root, 'debug')], {
+      onlyDirectories: true,
+    });
+    await config.update(
+      'linter.includePaths',
+      ['${workspaceFolder}/lint/**', path.join(root, 'debug')],
+      false
+    );
+    const paths = linter['getGlobPathsFromSettings']('linter.includePaths');
+    deepStrictEqual(paths, refs);
+  });
+
+  suiteTeardown(async function (): Promise<void> {
+    await config.update('linter.includePaths', oldVals, false);
+    // We have to reset the formatting of the files, for some reason updating
+    // the config breaks any the formatting style.
+    cp.spawn('npm', ['run', 'format'], { cwd: path.resolve(root, '../../') });
+  });
+});
+
+// -----------------------------------------------------------------------------
 
 suite('GNU (gfortran) lint single', () => {
   const linter = new FortranLintingProvider();