From f35daa7f15f08b666c6f242a0705e1592eea3c73 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Tue, 20 Feb 2024 13:05:53 -0500 Subject: [PATCH 1/8] fix(typescript-estree): use simpler absolutify behavior for project service client file paths --- .../src/useProgramFromProjectService.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/packages/typescript-estree/src/useProgramFromProjectService.ts b/packages/typescript-estree/src/useProgramFromProjectService.ts index d49acd55e095..205bf7a8192c 100644 --- a/packages/typescript-estree/src/useProgramFromProjectService.ts +++ b/packages/typescript-estree/src/useProgramFromProjectService.ts @@ -1,12 +1,9 @@ import { minimatch } from 'minimatch'; +import path from 'path'; import { createProjectProgram } from './create-program/createProjectProgram'; import type { ProjectServiceSettings } from './create-program/createProjectService'; -import { - type ASTAndDefiniteProgram, - ensureAbsolutePath, - getCanonicalFileName, -} from './create-program/shared'; +import { type ASTAndDefiniteProgram } from './create-program/shared'; import type { MutableParseSettings } from './parseSettings'; export function useProgramFromProjectService( @@ -14,10 +11,10 @@ export function useProgramFromProjectService( parseSettings: Readonly, hasFullTypeInformation: boolean, ): ASTAndDefiniteProgram | undefined { - const filePath = getCanonicalFileName(parseSettings.filePath); + const filePath = absolutify(parseSettings.filePath); const opened = service.openClientFile( - ensureAbsolutePath(filePath, service.host.getCurrentDirectory()), + filePath, parseSettings.codeFullText, /* scriptKind */ undefined, parseSettings.tsconfigRootDir, @@ -48,6 +45,12 @@ export function useProgramFromProjectService( } return createProjectProgram(parseSettings, [program]); + + function absolutify(filePath: string): string { + return path.isAbsolute(filePath) + ? filePath + : path.join(service.host.getCurrentDirectory(), filePath); + } } function filePathMatchedBy( From 75884ca953fd63836838ec1c74458a9539caefb1 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Tue, 20 Feb 2024 14:27:52 -0500 Subject: [PATCH 2/8] A bit of internal cleanup, and added unit tests --- .../src/useProgramFromProjectService.ts | 19 ++- .../lib/useProgramFromProjectService.test.ts | 140 ++++++++++++++++++ 2 files changed, 152 insertions(+), 7 deletions(-) create mode 100644 packages/typescript-estree/tests/lib/useProgramFromProjectService.test.ts diff --git a/packages/typescript-estree/src/useProgramFromProjectService.ts b/packages/typescript-estree/src/useProgramFromProjectService.ts index 205bf7a8192c..4f1c9bed0c17 100644 --- a/packages/typescript-estree/src/useProgramFromProjectService.ts +++ b/packages/typescript-estree/src/useProgramFromProjectService.ts @@ -11,30 +11,35 @@ export function useProgramFromProjectService( parseSettings: Readonly, hasFullTypeInformation: boolean, ): ASTAndDefiniteProgram | undefined { - const filePath = absolutify(parseSettings.filePath); + const filePathAbsolute = absolutify(parseSettings.filePath); const opened = service.openClientFile( - filePath, + filePathAbsolute, parseSettings.codeFullText, /* scriptKind */ undefined, parseSettings.tsconfigRootDir, ); if (hasFullTypeInformation) { + const isDefaultProjectAllowedPath = filePathMatchedBy( + parseSettings.filePath, + allowDefaultProjectForFiles, + ); + if (opened.configFileName) { - if (filePathMatchedBy(filePath, allowDefaultProjectForFiles)) { + if (isDefaultProjectAllowedPath) { throw new Error( - `${filePath} was included by allowDefaultProjectForFiles but also was found in the project service. Consider removing it from allowDefaultProjectForFiles.`, + `${parseSettings.filePath} was included by allowDefaultProjectForFiles but also was found in the project service. Consider removing it from allowDefaultProjectForFiles.`, ); } - } else if (!filePathMatchedBy(filePath, allowDefaultProjectForFiles)) { + } else if (!isDefaultProjectAllowedPath) { throw new Error( - `${filePath} was not found by the project service. Consider either including it in the tsconfig.json or including it in allowDefaultProjectForFiles.`, + `${parseSettings.filePath} was not found by the project service. Consider either including it in the tsconfig.json or including it in allowDefaultProjectForFiles.`, ); } } - const scriptInfo = service.getScriptInfo(filePath); + const scriptInfo = service.getScriptInfo(filePathAbsolute); const program = service .getDefaultProjectForFile(scriptInfo!.fileName, true)! .getLanguageService(/*ensureSynchronized*/ true) diff --git a/packages/typescript-estree/tests/lib/useProgramFromProjectService.test.ts b/packages/typescript-estree/tests/lib/useProgramFromProjectService.test.ts new file mode 100644 index 000000000000..69c05628f9fa --- /dev/null +++ b/packages/typescript-estree/tests/lib/useProgramFromProjectService.test.ts @@ -0,0 +1,140 @@ +/* eslint-disable @typescript-eslint/explicit-function-return-type -- Fancy mocks */ +import type { TypeScriptProjectService } from '../../src/create-program/createProjectService'; +import type { ParseSettings } from '../../src/parseSettings'; +import { useProgramFromProjectService } from '../../src/useProgramFromProjectService'; + +const mockCreateProjectProgram = jest.fn(); + +jest.mock('../../src/create-program/createProjectProgram', () => ({ + get createProjectProgram() { + return mockCreateProjectProgram; + }, +})); + +const currentDirectory = '/repos/repo'; + +function createMockProjectService() { + const openClientFile = jest.fn(); + const service = { + getDefaultProjectForFile: () => ({ + getLanguageService: () => ({ + getProgram: () => ({ + getSourceFile: () => ({}), + }), + }), + }), + getScriptInfo: () => ({}), + host: { + getCurrentDirectory: () => currentDirectory, + }, + openClientFile, + }; + + return { + service: service as typeof service & TypeScriptProjectService, + openClientFile, + }; +} + +const mockParseSettings = { + filePath: 'path/PascalCaseDirectory/camelCaseFile.ts', +} as ParseSettings; + +describe('useProgramFromProjectService', () => { + it('passes an absolute, case-matching file path to service.openClientFile', () => { + const { service } = createMockProjectService(); + + useProgramFromProjectService( + { allowDefaultProjectForFiles: undefined, service }, + mockParseSettings, + false, + ); + + expect(service.openClientFile).toHaveBeenCalledWith( + '/repos/repo/path/PascalCaseDirectory/camelCaseFile.ts', + undefined, + undefined, + undefined, + ); + }); + + it('throws an error when hasFullTypeInformation is enabled and the file is both in the project service and allowDefaultProjectForFiles', () => { + const { service } = createMockProjectService(); + + service.openClientFile.mockReturnValue({ configFileName: 'tsconfig.json' }); + + expect(() => + useProgramFromProjectService( + { allowDefaultProjectForFiles: [mockParseSettings.filePath], service }, + mockParseSettings, + true, + ), + ).toThrow( + `${mockParseSettings.filePath} was included by allowDefaultProjectForFiles but also was found in the project service. Consider removing it from allowDefaultProjectForFiles.`, + ); + }); + + it('throws an error when hasFullTypeInformation is enabled and the file is neither in the project service nor allowDefaultProjectForFiles', () => { + const { service } = createMockProjectService(); + + service.openClientFile.mockReturnValue({}); + + expect(() => + useProgramFromProjectService( + { allowDefaultProjectForFiles: [], service }, + mockParseSettings, + true, + ), + ).toThrow( + `${mockParseSettings.filePath} was not found by the project service. Consider either including it in the tsconfig.json or including it in allowDefaultProjectForFiles.`, + ); + }); + + it('returns undefined when hasFullTypeInformation is disabled, the file is both in the project service and allowDefaultProjectForFiles, and the service does not have a matching program', () => { + const { service } = createMockProjectService(); + + service.openClientFile.mockReturnValue({ configFileName: 'tsconfig.json' }); + + const actual = useProgramFromProjectService( + { allowDefaultProjectForFiles: [mockParseSettings.filePath], service }, + mockParseSettings, + false, + ); + + expect(actual).toBeUndefined(); + }); + + it('returns a created program when hasFullTypeInformation is disabled, the file is both in the project service and allowDefaultProjectForFiles, and the service has a matching program', () => { + const { service } = createMockProjectService(); + const program = { getSourceFile: jest.fn() }; + + service.openClientFile.mockReturnValue({ configFileName: 'tsconfig.json' }); + mockCreateProjectProgram.mockReturnValue(program); + + const actual = useProgramFromProjectService( + { allowDefaultProjectForFiles: [mockParseSettings.filePath], service }, + mockParseSettings, + false, + ); + + expect(actual).toBe(program); + }); + + it('returns a created program when hasFullTypeInformation is disabled, the file is neither in the project service nor allowDefaultProjectForFiles, and the service has a matching program', () => { + const { service } = createMockProjectService(); + const program = { getSourceFile: jest.fn() }; + + service.openClientFile.mockReturnValue({}); + mockCreateProjectProgram.mockReturnValue(program); + + const actual = useProgramFromProjectService( + { allowDefaultProjectForFiles: [], service }, + mockParseSettings, + false, + ); + + expect(actual).toBe(program); + }); +}); + +/* eslint-enable @typescript-eslint/explicit-function-return-type */ From ddb17c75e2e8bd4449a9cfa433aeadfcb2f4565c Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Tue, 20 Feb 2024 20:48:47 -0500 Subject: [PATCH 3/8] Lol Windows --- .../tests/lib/useProgramFromProjectService.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/typescript-estree/tests/lib/useProgramFromProjectService.test.ts b/packages/typescript-estree/tests/lib/useProgramFromProjectService.test.ts index 69c05628f9fa..b251c801820a 100644 --- a/packages/typescript-estree/tests/lib/useProgramFromProjectService.test.ts +++ b/packages/typescript-estree/tests/lib/useProgramFromProjectService.test.ts @@ -1,4 +1,6 @@ /* eslint-disable @typescript-eslint/explicit-function-return-type -- Fancy mocks */ +import path from 'path'; + import type { TypeScriptProjectService } from '../../src/create-program/createProjectService'; import type { ParseSettings } from '../../src/parseSettings'; import { useProgramFromProjectService } from '../../src/useProgramFromProjectService'; @@ -51,7 +53,7 @@ describe('useProgramFromProjectService', () => { ); expect(service.openClientFile).toHaveBeenCalledWith( - '/repos/repo/path/PascalCaseDirectory/camelCaseFile.ts', + path.normalize('/repos/repo/path/PascalCaseDirectory/camelCaseFile.ts'), undefined, undefined, undefined, @@ -136,5 +138,3 @@ describe('useProgramFromProjectService', () => { expect(actual).toBe(program); }); }); - -/* eslint-enable @typescript-eslint/explicit-function-return-type */ From c4bfec04c7629bc9884fb4cbef0d55d879586010 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Wed, 21 Feb 2024 07:07:12 -0500 Subject: [PATCH 4/8] Standardize type specifier style --- packages/typescript-estree/src/useProgramFromProjectService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/typescript-estree/src/useProgramFromProjectService.ts b/packages/typescript-estree/src/useProgramFromProjectService.ts index 4f1c9bed0c17..801eda79b141 100644 --- a/packages/typescript-estree/src/useProgramFromProjectService.ts +++ b/packages/typescript-estree/src/useProgramFromProjectService.ts @@ -3,7 +3,7 @@ import path from 'path'; import { createProjectProgram } from './create-program/createProjectProgram'; import type { ProjectServiceSettings } from './create-program/createProjectService'; -import { type ASTAndDefiniteProgram } from './create-program/shared'; +import type { ASTAndDefiniteProgram } from './create-program/shared'; import type { MutableParseSettings } from './parseSettings'; export function useProgramFromProjectService( From 3626af57dae88d1e36b0f61af4714086fa42ef9a Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Fri, 23 Feb 2024 07:57:14 -0500 Subject: [PATCH 5/8] oopity --- packages/typescript-estree/src/useProgramFromProjectService.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/typescript-estree/src/useProgramFromProjectService.ts b/packages/typescript-estree/src/useProgramFromProjectService.ts index a7ed0a94d38d..ee93fc24d7b2 100644 --- a/packages/typescript-estree/src/useProgramFromProjectService.ts +++ b/packages/typescript-estree/src/useProgramFromProjectService.ts @@ -44,6 +44,7 @@ export function useProgramFromProjectService( log( 'Default project allowed path: %s, based on config file: %s', + isDefaultProjectAllowedPath, opened.configFileName, ); From 5dabf6204af3d08f3e5ff818fbd889a435977765 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Fri, 23 Feb 2024 09:34:52 -0500 Subject: [PATCH 6/8] Correct unit test for undefined --- .../lib/useProgramFromProjectService.test.ts | 32 +++++++++++++------ 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/packages/typescript-estree/tests/lib/useProgramFromProjectService.test.ts b/packages/typescript-estree/tests/lib/useProgramFromProjectService.test.ts index b251c801820a..cec730d1cf74 100644 --- a/packages/typescript-estree/tests/lib/useProgramFromProjectService.test.ts +++ b/packages/typescript-estree/tests/lib/useProgramFromProjectService.test.ts @@ -13,6 +13,8 @@ jest.mock('../../src/create-program/createProjectProgram', () => ({ }, })); +const mockGetProgram = jest.fn(); + const currentDirectory = '/repos/repo'; function createMockProjectService() { @@ -20,9 +22,7 @@ function createMockProjectService() { const service = { getDefaultProjectForFile: () => ({ getLanguageService: () => ({ - getProgram: () => ({ - getSourceFile: () => ({}), - }), + getProgram: mockGetProgram, }), }), getScriptInfo: () => ({}), @@ -63,7 +63,9 @@ describe('useProgramFromProjectService', () => { it('throws an error when hasFullTypeInformation is enabled and the file is both in the project service and allowDefaultProjectForFiles', () => { const { service } = createMockProjectService(); - service.openClientFile.mockReturnValue({ configFileName: 'tsconfig.json' }); + service.openClientFile.mockReturnValueOnce({ + configFileName: 'tsconfig.json', + }); expect(() => useProgramFromProjectService( @@ -79,7 +81,7 @@ describe('useProgramFromProjectService', () => { it('throws an error when hasFullTypeInformation is enabled and the file is neither in the project service nor allowDefaultProjectForFiles', () => { const { service } = createMockProjectService(); - service.openClientFile.mockReturnValue({}); + service.openClientFile.mockReturnValueOnce({}); expect(() => useProgramFromProjectService( @@ -95,7 +97,11 @@ describe('useProgramFromProjectService', () => { it('returns undefined when hasFullTypeInformation is disabled, the file is both in the project service and allowDefaultProjectForFiles, and the service does not have a matching program', () => { const { service } = createMockProjectService(); - service.openClientFile.mockReturnValue({ configFileName: 'tsconfig.json' }); + mockGetProgram.mockReturnValue(undefined); + + service.openClientFile.mockReturnValueOnce({ + configFileName: 'tsconfig.json', + }); const actual = useProgramFromProjectService( { allowDefaultProjectForFiles: [mockParseSettings.filePath], service }, @@ -110,8 +116,12 @@ describe('useProgramFromProjectService', () => { const { service } = createMockProjectService(); const program = { getSourceFile: jest.fn() }; - service.openClientFile.mockReturnValue({ configFileName: 'tsconfig.json' }); - mockCreateProjectProgram.mockReturnValue(program); + mockGetProgram.mockReturnValue(program); + + service.openClientFile.mockReturnValueOnce({ + configFileName: 'tsconfig.json', + }); + mockCreateProjectProgram.mockReturnValueOnce(program); const actual = useProgramFromProjectService( { allowDefaultProjectForFiles: [mockParseSettings.filePath], service }, @@ -126,8 +136,10 @@ describe('useProgramFromProjectService', () => { const { service } = createMockProjectService(); const program = { getSourceFile: jest.fn() }; - service.openClientFile.mockReturnValue({}); - mockCreateProjectProgram.mockReturnValue(program); + mockGetProgram.mockReturnValue(program); + + service.openClientFile.mockReturnValueOnce({}); + mockCreateProjectProgram.mockReturnValueOnce(program); const actual = useProgramFromProjectService( { allowDefaultProjectForFiles: [], service }, From 65685cea5b07f7652b6c70bb430890660d3a39df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josh=20Goldberg=20=E2=9C=A8?= Date: Fri, 23 Feb 2024 22:57:33 -0500 Subject: [PATCH 7/8] Update packages/typescript-estree/src/useProgramFromProjectService.ts Co-authored-by: Brad Zacher --- packages/typescript-estree/src/useProgramFromProjectService.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/typescript-estree/src/useProgramFromProjectService.ts b/packages/typescript-estree/src/useProgramFromProjectService.ts index ee93fc24d7b2..010fdb6e39ce 100644 --- a/packages/typescript-estree/src/useProgramFromProjectService.ts +++ b/packages/typescript-estree/src/useProgramFromProjectService.ts @@ -16,6 +16,7 @@ export function useProgramFromProjectService( parseSettings: Readonly, hasFullTypeInformation: boolean, ): ASTAndDefiniteProgram | undefined { + // we don't use canonicalized filename here because it causes massive perf regressions for some reason const filePathAbsolute = absolutify(parseSettings.filePath); log( 'Opening project service file for: %s at absolute path %s', From 898b1dfd1369fdfedbf62f3321d92719fed8cf3c Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 24 Feb 2024 08:23:20 -0500 Subject: [PATCH 8/8] cspell --- .cspell.json | 1 + packages/typescript-estree/src/useProgramFromProjectService.ts | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/.cspell.json b/.cspell.json index 01235a662ad8..93e4a9b6594d 100644 --- a/.cspell.json +++ b/.cspell.json @@ -66,6 +66,7 @@ "blurple", "bradzacher", "camelcase", + "canonicalize", "Cena", "codebases", "Codecov", diff --git a/packages/typescript-estree/src/useProgramFromProjectService.ts b/packages/typescript-estree/src/useProgramFromProjectService.ts index 010fdb6e39ce..9ceb2d210c11 100644 --- a/packages/typescript-estree/src/useProgramFromProjectService.ts +++ b/packages/typescript-estree/src/useProgramFromProjectService.ts @@ -16,7 +16,8 @@ export function useProgramFromProjectService( parseSettings: Readonly, hasFullTypeInformation: boolean, ): ASTAndDefiniteProgram | undefined { - // we don't use canonicalized filename here because it causes massive perf regressions for some reason + // We don't canonicalize the filename because it caused a performance regression. + // See https://github.com/typescript-eslint/typescript-eslint/issues/8519 const filePathAbsolute = absolutify(parseSettings.filePath); log( 'Opening project service file for: %s at absolute path %s',