diff --git a/.eslintrc.js b/.eslintrc.js index c9e8f08ef..93842dca9 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -221,6 +221,7 @@ module.exports = { '@typescript-eslint/no-unused-vars-experimental': 'off', '@typescript-eslint/dot-notation': 'off', '@typescript-eslint/no-unsafe-argument': 'off', + 'func-names': 'off', 'new-cap': 'off', 'no-shadow': 'off', 'no-void': 'off' diff --git a/src/LanguageServer.spec.ts b/src/LanguageServer.spec.ts index 9bcebcbd5..77a770c31 100644 --- a/src/LanguageServer.spec.ts +++ b/src/LanguageServer.spec.ts @@ -11,7 +11,7 @@ import { TextDocument } from 'vscode-languageserver-textdocument'; import type { Program } from './Program'; import * as assert from 'assert'; import type { PartialDiagnostic } from './testHelpers.spec'; -import { expectZeroDiagnostics, normalizeDiagnostics, trim } from './testHelpers.spec'; +import { createInactivityStub, expectZeroDiagnostics, normalizeDiagnostics, trim } from './testHelpers.spec'; import { isBrsFile, isLiteralString } from './astUtils/reflection'; import { createVisitor, WalkMode } from './astUtils/visitors'; import { tempDir, rootDir } from './testHelpers.spec'; @@ -23,6 +23,7 @@ import { LogLevel, Logger, createLogger } from './logging'; import { DiagnosticMessages } from './DiagnosticMessages'; import { standardizePath } from 'roku-deploy'; import undent from 'undent'; +import { ProjectManager } from './lsp/ProjectManager'; const sinon = createSandbox(); @@ -84,6 +85,8 @@ describe('LanguageServer', () => { beforeEach(() => { sinon.restore(); + fsExtra.emptyDirSync(tempDir); + server = new LanguageServer(); server['busyStatusTracker'] = new BusyStatusTracker(); workspaceFolders = [workspacePath]; @@ -95,9 +98,11 @@ describe('LanguageServer', () => { }); server['hasConfigurationCapability'] = true; }); + afterEach(() => { sinon.restore(); fsExtra.emptyDirSync(tempDir); + server['dispose'](); LanguageServer.enableThreadingDefault = enableThreadingDefault; }); @@ -123,6 +128,44 @@ describe('LanguageServer', () => { } } + it('does not cause infinite loop of project creation', async () => { + //add a project with a files array that includes (and then excludes) a file + fsExtra.outputFileSync(s`${rootDir}/bsconfig.json`, JSON.stringify({ + files: ['source/**/*', '!source/**/*.spec.bs'] + })); + + server['run'](); + + function setSyncedDocument(srcPath: string, text: string, version = 1) { + //force an open text document + const document = TextDocument.create(util.pathToUri( + util.standardizePath(srcPath + ) + ), 'brightscript', 1, `sub main()\nend sub`); + (server['documents']['_syncedDocuments'] as Map).set(document.uri, document); + } + + //wait for the projects to finish loading up + await server['syncProjects'](); + + //this bug was causing an infinite async loop of new project creations. So monitor the creation of new projects for evaluation later + const { stub, promise: createProjectsSettled } = createInactivityStub(ProjectManager.prototype as any, 'constructProject', 400, sinon); + + setSyncedDocument(s`${rootDir}/source/lib1.spec.bs`, 'sub lib1()\nend sub'); + setSyncedDocument(s`${rootDir}/source/lib2.spec.bs`, 'sub lib2()\nend sub'); + + // open a file that is excluded by the project, so it should trigger a standalone project. + await server['onTextDocumentDidChangeContent']({ + document: TextDocument.create(util.pathToUri(s`${rootDir}/source/lib1.spec.bs`), 'brightscript', 1, `sub main()\nend sub`) + }); + + //wait for the "create projects" deferred debounce to settle + await createProjectsSettled; + + //test passes if we've only made 2 new projects (one for each of the standalone projects) + expect(stub.callCount).to.eql(2); + }); + describe('onDidChangeConfiguration', () => { async function doTest(startingConfigs: WorkspaceConfigWithExtras[], endingConfigs: WorkspaceConfigWithExtras[]) { (server as any)['connection'] = connection; @@ -637,6 +680,7 @@ describe('LanguageServer', () => { expect( filterer.filter([ s`${workspacePath}/src/source/file.brs`, + //this file should be excluded s`${workspacePath}/dist/source/file.brs` ]) ).to.eql([ diff --git a/src/LanguageServer.ts b/src/LanguageServer.ts index 162bc375f..030661112 100644 --- a/src/LanguageServer.ts +++ b/src/LanguageServer.ts @@ -135,7 +135,6 @@ export class LanguageServer { }); this.projectManager.busyStatusTracker.on('active-runs-change', (event) => { - console.log(event); this.sendBusyStatus(); }); } diff --git a/src/lsp/PathFilterer.spec.ts b/src/lsp/PathFilterer.spec.ts index 2c76a7eb6..2e1b96a65 100644 --- a/src/lsp/PathFilterer.spec.ts +++ b/src/lsp/PathFilterer.spec.ts @@ -178,3 +178,47 @@ describe('PathFilterer', () => { }); }); }); + +describe('PathCollection', () => { + function doTest(globs: string[], filePath: string, expected: boolean) { + const collection = new PathCollection({ + rootDir: rootDir, + globs: globs + }); + expect(collection.isMatch(filePath)).to.equal(expected); + } + + it('includes a file that matches a single pattern', () => { + doTest([ + '**/*.brs' + ], s`${rootDir}/alpha.brs`, true); + }); + + it('includes a file that matches the 2nd pattern', () => { + doTest([ + '**/*beta*.brs', + '**/*alpha*.brs' + ], s`${rootDir}/alpha.brs`, true); + }); + + it('includes a file that is included then excluded then included again', () => { + doTest([ + '**/*.brs', + '!**/a*.brs', + '**/alpha.brs' + ], s`${rootDir}/alpha.brs`, true); + }); + + it('excludes a file that does not match the pattern', () => { + doTest([ + '**/beta.brs' + ], s`${rootDir}/alpha.brs`, false); + }); + + it('excludes a file that matches first pattern but is excluded from the second pattern', () => { + doTest([ + '**/*.brs', + '!**/alpha.brs' + ], s`${rootDir}/alpha.brs`, false); + }); +}); diff --git a/src/lsp/PathFilterer.ts b/src/lsp/PathFilterer.ts index 61de34fbe..45e79009b 100644 --- a/src/lsp/PathFilterer.ts +++ b/src/lsp/PathFilterer.ts @@ -133,7 +133,8 @@ export class PathFilterer { this.logger.debug('registerExcludeMatcher', matcher); const collection = new PathCollection({ - matcher: matcher + matcher: matcher, + isExcludePattern: false }); this.excludeCollections.push(collection); return () => { @@ -168,33 +169,55 @@ export class PathCollection { globs: string[]; } | { matcher: (path: string) => boolean; + isExcludePattern: boolean; } ) { if ('globs' in options) { //build matcher patterns from the globs - for (const glob of options.globs ?? []) { + for (let glob of options.globs ?? []) { + let isExcludePattern = glob.startsWith('!'); + if (isExcludePattern) { + glob = glob.substring(1); + } const pattern = path.resolve( options.rootDir, glob ).replace(/\\+/g, '/'); - this.matchers.push( - micromatch.matcher(pattern) - ); + this.matchers.push({ + pattern: pattern, + isMatch: micromatch.matcher(pattern), + isExcludePattern: isExcludePattern + }); } } else { - this.matchers.push(options.matcher); + this.matchers.push({ + isMatch: options.matcher, + isExcludePattern: options.isExcludePattern + }); } } - private matchers: Array<(string) => boolean> = []; + private matchers: Array<{ + pattern?: string; + isMatch: (string) => boolean; + isExcludePattern: boolean; + }> = []; + public isMatch(path: string) { + let keep = false; //coerce the path into a normalized form and unix slashes path = util.standardizePath(path).replace(/\\+/g, '/'); for (let matcher of this.matchers) { - if (matcher(path)) { - return true; + //exclusion pattern: do not keep the path if it matches + if (matcher.isExcludePattern) { + if (matcher.isMatch(path)) { + keep = false; + } + //inclusion pattern: keep the path if it matches + } else { + keep = keep || matcher.isMatch(path); } } - return false; + return keep; } } diff --git a/src/lsp/Project.spec.ts b/src/lsp/Project.spec.ts index 0d1a06173..3298f5c28 100644 --- a/src/lsp/Project.spec.ts +++ b/src/lsp/Project.spec.ts @@ -70,7 +70,7 @@ describe('Project', () => { }); }); - describe('setFile', () => { + describe('applyFileChanges', () => { it('skips setting the file if the contents have not changed', async () => { await project.activate({ projectPath: rootDir } as any); //initial set should be true @@ -100,6 +100,43 @@ describe('Project', () => { }]))[0].status ).to.eql('accepted'); }); + + it('always includes a status', async () => { + await project.activate({ + projectPath: rootDir + } as any); + + project['builder'].options.files = [ + 'source/**/*', + '!source/**/*.spec.bs' + ]; + + //set file that maches files array + expect((await project['applyFileChanges']([{ + fileContents: '', + srcPath: s`${rootDir}/source/main.bs`, + type: 'set' + }]))[0].status).to.eql('accepted'); + + //delete this file that matches a file in the program + expect((await project['applyFileChanges']([{ + srcPath: s`${rootDir}/source/main.bs`, + type: 'delete' + }]))[0].status).to.eql('accepted'); + + //set file that does not match files array files array + expect((await project['applyFileChanges']([{ + fileContents: '', + srcPath: s`${rootDir}/source/main.spec.bs`, + type: 'set' + }]))[0].status).to.eql('rejected'); + + //delete directory is "reject" because those should be unraveled into individual files on the outside + expect((await project['applyFileChanges']([{ + srcPath: s`${rootDir}/source`, + type: 'delete' + }]))[0].status).to.eql('rejected'); + }); }); describe('activate', () => { diff --git a/src/lsp/Project.ts b/src/lsp/Project.ts index 2827007ea..47743e33a 100644 --- a/src/lsp/Project.ts +++ b/src/lsp/Project.ts @@ -227,7 +227,7 @@ export class Project implements LspProject { const action = result[i]; let didChangeThisFile = false; //if this is a `set` and the file matches the project's files array, set it - if (action.type === 'set' && this.willAcceptFile(action.srcPath)) { + if (action.type === 'set' && Project.willAcceptFile(action.srcPath, this.builder.program.options.files, this.builder.program.options.rootDir)) { //load the file contents from disk if we don't have an in memory copy const fileContents = action.fileContents ?? util.readFileSync(action.srcPath)?.toString(); @@ -255,6 +255,10 @@ export class Project implements LspProject { didChangeThisFile = this.removeFileOrDirectory(action.srcPath); //if we deleted at least one file, mark this action as accepted action.status = didChangeThisFile ? 'accepted' : 'rejected'; + + //we did not handle this action, so reject + } else { + action.status = 'rejected'; } didChangeFiles = didChangeFiles || didChangeThisFile; } @@ -271,12 +275,12 @@ export class Project implements LspProject { /** * Determine if this project will accept the file at the specified path (i.e. does it match a pattern in the project's files array) */ - private willAcceptFile(srcPath: string) { + public static willAcceptFile(srcPath: string, files: BsConfig['files'], rootDir: string) { srcPath = util.standardizePath(srcPath); - if (rokuDeploy.getDestPath(srcPath, this.builder.program.options.files, this.builder.program.options.rootDir) !== undefined) { + if (rokuDeploy.getDestPath(srcPath, files, rootDir) !== undefined) { return true; //is this exact path in the `files` array? (this check is mostly for standalone projects) - } else if ((this.builder.program.options.files as StandardizedFileEntry[]).find(x => s`${x.src}` === srcPath)) { + } else if ((files as StandardizedFileEntry[]).find(x => s`${x.src}` === srcPath)) { return true; } return false; diff --git a/src/lsp/ProjectManager.spec.ts b/src/lsp/ProjectManager.spec.ts index f79af0a29..045d3024f 100644 --- a/src/lsp/ProjectManager.spec.ts +++ b/src/lsp/ProjectManager.spec.ts @@ -513,7 +513,7 @@ describe('ProjectManager', () => { await onNextDiagnostics(); //there should NOT be a standalone project - expect(manager['standaloneProjects'].length).to.eql(0); + expect(manager['standaloneProjects'].size).to.eql(0); }); it('converts a missing file to a delete', async () => { @@ -740,13 +740,15 @@ describe('ProjectManager', () => { allowStandaloneProject: true }]); await onNextDiagnostics(); - expect(manager['standaloneProjects'][0]?.srcPath).to.eql(s`${rootDir}/source/main.brs`); + expect( + [...manager['standaloneProjects'].values()][0]?.srcPath + ).to.eql(s`${rootDir}/source/main.brs`); //it deletes the standalone project when the file is closed await manager.handleFileClose({ srcPath: `${rootDir}/source/main.brs` }); - expect(manager['standaloneProjects']).to.be.empty; + expect(manager['standaloneProjects'].size).to.eql(0); }); }); }); diff --git a/src/lsp/ProjectManager.ts b/src/lsp/ProjectManager.ts index 362defdf0..bf63e3361 100644 --- a/src/lsp/ProjectManager.ts +++ b/src/lsp/ProjectManager.ts @@ -19,6 +19,12 @@ import { createLogger } from '../logging'; import { Cache } from '../Cache'; import { ActionQueue } from './ActionQueue'; +const FileChangeTypeLookup = Object.entries(FileChangeType).reduce((acc, [key, value]) => { + acc[value] = key; + acc[key] = value; + return acc; +}, {}); + /** * Manages all brighterscript projects for the language server */ @@ -57,7 +63,7 @@ export class ProjectManager { * Collection of standalone projects. These are projects that are not part of a workspace, but are instead single files. * All of these are also present in the `projects` collection. */ - private standaloneProjects: StandaloneProject[] = []; + private standaloneProjects = new Map(); private documentManager: DocumentManager; public static documentManagerDelay = 150; @@ -99,7 +105,12 @@ export class ProjectManager { }); // only include files that are applicable to this specific project (still allow deletes to flow through since they're cheap) const projectActions = actions.filter(action => { - return action.type === 'delete' || filterer.isMatch(action.srcPath); + return ( + //if this is a delete, just pass it through because they're cheap to apply + action.type === 'delete' || + //if this is a set, only pass it through if it's a file that this project cares about + filterer.isMatch(action.srcPath) + ); }); if (projectActions.length > 0) { const responseActions = await project.applyFileChanges(projectActions); @@ -122,29 +133,38 @@ export class ProjectManager { const handledResponses = flatResponses.filter(x => x?.action?.id === action.id && x?.action?.status === 'accepted'); //remove any standalone project created for this file since it was handled by a normal project - if (handledResponses.some(x => x.project.isStandaloneProject === false)) { - this.removeStandaloneProject(action.srcPath); + const normalProjectsThatHandledThisFile = handledResponses.filter(x => !x.project.isStandaloneProject); + if (normalProjectsThatHandledThisFile.length > 0) { + //if there's a standalone project for this file, delete it + if (this.getStandaloneProject(action.srcPath, false)) { + this.logger.debug( + `flushDocumentChanges: removing standalone project because the following normal projects handled the file: '${action.srcPath}', projects:`, + normalProjectsThatHandledThisFile.map(x => x.project.projectIdentifier) + ); + this.removeStandaloneProject(action.srcPath); + } // create a standalone project if this action was handled by zero normal projects. - //(save to call even if there's already a standalone project, won't create dupes) + //(safe to call even if there's already a standalone project, won't create dupes) } else { //TODO only create standalone projects for files we understand (brightscript, brighterscript, scenegraph xml, etc) await this.createStandaloneProject(action.srcPath); } - this.logger.info('flushDocumentChanges complete', actions.map(x => ({ - type: x.type, - srcPath: x.srcPath, - allowStandaloneProject: x.allowStandaloneProject - }))); } + this.logger.info('flushDocumentChanges complete', actions.map(x => ({ + type: x.type, + srcPath: x.srcPath, + allowStandaloneProject: x.allowStandaloneProject + }))); } /** * Get a standalone project for a given file path */ - private getStandaloneProject(srcPath: string) { - srcPath = util.standardizePath(srcPath); - return this.standaloneProjects.find(x => x.srcPath === srcPath); + private getStandaloneProject(srcPath: string, standardizePath = true) { + return this.standaloneProjects.get( + standardizePath ? util.standardizePath(srcPath) : srcPath + ); } /** @@ -154,7 +174,7 @@ export class ProjectManager { srcPath = util.standardizePath(srcPath); //if we already have a standalone project with this path, do nothing because it already exists - if (this.getStandaloneProject(srcPath)) { + if (this.getStandaloneProject(srcPath, false)) { this.logger.log('createStandaloneProject skipping because we already have one for this path'); return; } @@ -179,18 +199,18 @@ export class ProjectManager { project.srcPath = srcPath; project.isStandaloneProject = true; - this.standaloneProjects.push(project); + this.standaloneProjects.set(srcPath, project); await this.activateProject(project, projectOptions); } private removeStandaloneProject(srcPath: string) { srcPath = util.standardizePath(srcPath); - //remove all standalone projects that have this srcPath - for (let i = this.standaloneProjects.length - 1; i >= 0; i--) { - const project = this.standaloneProjects[i]; + const project = this.getStandaloneProject(srcPath, false); + if (project) { if (project.srcPath === srcPath) { + this.logger.debug(`Removing standalone project for file '${srcPath}'`); this.removeProject(project); - this.standaloneProjects.splice(i, 1); + this.standaloneProjects.delete(srcPath); } } } @@ -312,7 +332,7 @@ export class ProjectManager { }); public handleFileChanges(changes: FileChange[]) { - this.logger.debug('handleFileChanges', changes.map(x => `${FileChangeType[x.type]}: ${x.srcPath}`)); + this.logger.debug('handleFileChanges', changes.map(x => `${FileChangeTypeLookup[x.type]}: ${x.srcPath}`)); //this function should NOT be marked as async, because typescript wraps the body in an async call sometimes. These need to be registered synchronously return this.fileChangesQueue.run(async (changes) => { @@ -331,7 +351,7 @@ export class ProjectManager { //filter any changes that are not allowed by the path filterer changes = this.pathFilterer.filter(changes, x => x.srcPath); - this.logger.debug('handleFileChanges -> filtered', changes.map(x => `${FileChangeType[x.type]}: ${x.srcPath}`)); + this.logger.debug('handleFileChanges -> filtered', changes.map(x => `${FileChangeTypeLookup[x.type]}: ${x.srcPath}`)); //process all file changes in parallel await Promise.all(changes.map(async (change) => { @@ -378,15 +398,18 @@ export class ProjectManager { //reload any projects whose bsconfig.json was changed const projectsToReload = this.projects.filter(x => x.bsconfigPath?.toLowerCase() === change.srcPath.toLowerCase()); - await Promise.all( - projectsToReload.map(x => this.reloadProject(x)) - ); + if (projectsToReload.length > 0) { + await Promise.all( + projectsToReload.map(x => this.reloadProject(x)) + ); + } } /** * Handle when a file is closed in the editor (this mostly just handles removing standalone projects) */ public async handleFileClose(event: { srcPath: string }) { + this.logger.debug(`File was closed. ${event.srcPath}`); this.removeStandaloneProject(event.srcPath); //most other methods on this class are async, might as well make this one async too for consistency and future expansion await Promise.resolve(); @@ -761,6 +784,10 @@ export class ProjectManager { @TrackBusyStatus private async activateProject(project: LspProject, config: ProjectConfig) { + this.logger.debug('Activating project', project.projectIdentifier, { + projectPath: config?.projectPath, + bsconfigPath: config.bsconfigPath + }); await project.activate(config); //send an event to indicate that this project has been activated diff --git a/src/testHelpers.spec.ts b/src/testHelpers.spec.ts index c049e07d8..926aa6e42 100644 --- a/src/testHelpers.spec.ts +++ b/src/testHelpers.spec.ts @@ -433,3 +433,41 @@ export async function onCalledOnce(thing: T, method: K): P }); }); } + + +/** + * Create a stub that resolves a promise after the function has settled for a period of time + */ +export function createInactivityStub(obj: T, methodName: keyof T, inactivityPeriod = 400, sinonRef = sinon) { + // Store reference to the original method + const originalMethod = obj[methodName]; + + // Track the timeout for inactivity + let inactivityTimeout; + + // Create the inactivity promise + let inactivityPromiseResolve; + const inactivityPromise = new Promise((resolve) => { + inactivityPromiseResolve = resolve; + }); + + // Stub the method + const stub = sinonRef.stub(obj, methodName).callsFake(function (this: any, ...args) { + // Call the original method + const result = (originalMethod as any).apply(this, args); + + // Clear previous inactivity timeout and restart the timer + clearTimeout(inactivityTimeout); + inactivityTimeout = setTimeout(() => { + inactivityPromiseResolve(); + }, inactivityPeriod); + + return result; + }); + + // Return the stub and inactivity promise + return { + stub: stub, + promise: inactivityPromise + }; +}