allegro / typescript-strict-plugin

Typescript plugin that allows turning on strict mode in specific files or directories.
MIT License
338 stars 29 forks source link

Use in-memory program to generate strict diagnostics for plugin #62

Closed hnra closed 9 months ago

hnra commented 10 months ago

The PR changes the strategy used to generate the strict diagnostics in the plugin by keeping an in-memory version of the TypeScript program instead of mutating the existing program used by the language service.

The in-memory version is a SemanticDiagnosticsBuilderProgram which the same type used in the TypeScript API example "Writing an incremental program watcher". This program type is partially internal since it requires that SourceFiles produced by the compiler host are versioned (which is an internal property). Luckily the language service produces versioned source files of the files currently being edited. I have only found one case where this does not happen which is when importing from reference projects, in particular it seems the language service does not keep track of the declaration files of referenced projects. In this case we fallback to the standard compiler host and generate a file version using a hashing algorithm which is also used as a fallback in the TypeScript repo.

I have tried to document assumptions made. This PR does not contain any tests and I have not looked at the non-plugin code to see if there is any possibility for code sharing. I have mainly implemented this to resolve https://github.com/allegro/typescript-strict-plugin/issues/61 for a project where I introduced this plugin (and would be very sad to lose it).

Fixes: https://github.com/allegro/typescript-strict-plugin/issues/61

KostkaBrukowa commented 10 months ago

I've released beta version with this code https://www.npmjs.com/package/typescript-strict-plugin?activeTab=readme and I will test on my repo if this works. Thanks for contribution

KostkaBrukowa commented 10 months ago

Unfortunately with this change I have this view in my VSCode. Tested with typescript@4.8.4 and typescript@5.3.3

image
hnra commented 10 months ago

Do you have any minimal repro steps? I tried the version you released the follow way:

npm init -y
npm install typescript@4.8.4 typescript-strict-plugin@2.2.2-beta.1 @types/node

tsconfig.json:

{
    "compilerOptions": {
        "strict": false,
        "plugins": [
            {
                "name": "typescript-strict-plugin"
            }
        ]
    }
}

index.ts:

import { resolve } from "path";

let foo: number | undefined = 42;
foo = undefined;

resolve(".");

Then:

  1. Open VSCode (1.85.0) using code .
  2. Select TypeScript Version > Use Workspace Version 4.8.4

Result: No errors as expected. If I remove | undefined I get a strictness error. If I add // @ts-strict-ignore the error is ignored.

I have not tried the released version yet on our 100k+ LOC repo due to Christmas holidays so can't comment on that yet.

KostkaBrukowa commented 10 months ago

I've tried on my 100k+ LOC project, so I don't have repro steps, but I will try to find how to reproduce it

KostkaBrukowa commented 10 months ago

Ok, i think I found it. I think that now the plugin reports errors from all of the files in the project. From you example you can add index2.ts with simillar file contents (but different lines)

import { resolve } from "path";
// 
//
let foo2: number | undefined = 42;
foo2 = undefined;

resolve(".");

and then, in index.ts file there would be an error

hnra commented 10 months ago

So I added the a file index2.ts with the same contents and changed index.ts such that it produces an error (removing | undefined), but I do not get the same behavior (see screenshot). Is the issue reproducible with the vanilla version of latest vscode?

However, the issue you are having should occur if getSourceFile returns undefined since that will result in getSemanticDiagnostics returning all project errors. I should add some robustness and logging around these parts, e.g. fallback to the language service if getSourceFile returns undefined. But if it's returning undefined for index2.ts then I'm guessing something has gone wrong with either createSemanticDiagnosticsBuilderProgram or the creation of the compiler host.

Screenshot from 2023-12-29 13-04-41

KostkaBrukowa commented 10 months ago

It's my VSCode view. You can

image image

My package.json

{
  "name": "test",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "dependencies": {
    "typescript": "5.3.3",
    "typescript-strict-plugin": "2.2.2-beta.1"
  },
  "keywords": [],
  "author": "",
  "license": "ISC"
}
hnra commented 10 months ago

Our project setup looks the same and we have the same version of VSCode. Maybe due to OS (Linux vs macOS) or how vscode is setup? FWIW I cannot reproduce the issue in neovim (using typescript-tools) either.

KostkaBrukowa commented 10 months ago

I'm using macOS and I can reproduce it in neovim using typescript-tools as well :( Very weird. I will try to debug what is happening inside the plugin itself

KostkaBrukowa commented 10 months ago

Hmmm... In my case I'm always getting SourceFile as undefined

Info 85   [13:27:37.275] [typescript-strict-plugin]: Test SourceFile: undefined, Path: /Users/.../Documents/Work/test/index.ts, languageVersionOrOptions: undefined
Info 86   [13:27:37.275] [typescript-strict-plugin]: Could not obtain SourceFile from language service and 'languageVerisionOrOptions' is not set for path '/Users/.../Documents/Work/test/index.ts'
Info 87   [13:27:37.275] [typescript-strict-plugin]: Could not obtain SourceFile for file with path '/Users/.../Documents/Work/test/index.ts'
heyimalex commented 9 months ago

I'm seeing the same behavior and logs for for the beta version on on mac. For anyone else who maybe wants to contribute:

A plugin is really a PluginModuleFactory. The interface works through decorating a LanguageService through a create function which gets passed a PluginCreateInfo.

Just thinking about this idea of maintaining two parallel language services and switching between diagnostics in the proxy. The LanguageService interface seems like it only has getters, so we can't just proxy calls on the language service to another instance, intercept compiler options calls to add strict or something. Looking at createLanguageService maybe we could do that through LanguageServiceHost though? Seems like that's how extensions interact with the language service. So something like:

function create(info) {
  // Proxy LanguageServiceHost to override compilation settings.
  const strictHost = {
    ...info.languageServiceHost,
    getCompilationSettings: () => ({
      ...info.languageServiceHost.getCompilcationSettings(),
      strict: true,
    }),
  };
  const strictLanguageServer = ts.createLanguageServer(strictHost);

  proxy.getSemanticDiagnostics = function (filePath) {
    const strictFile = new PluginStrictFileChecker(info).isFileStrict(filePath);

    if (strictFile) {
      return strictLanguageServer.getSemanticDiagnostics(filePath);
    } else {
      return info.languageService.getSemanticDiagnostics(filePath);
    }
  };
  // Maybe also forward cleanupSemanticCache and dispose calls.
}

Just spitballing. Will play around with it more if I've got time.

heyimalex commented 9 months ago

Tested my approach in a branch here. Needs some more work, but looking promising! Tested in my work repo and it's working well.

Screenshot 2024-01-23 at 8 18 49 PM
hnra commented 9 months ago

Closing this since https://github.com/allegro/typescript-strict-plugin/pull/67 fixes https://github.com/allegro/typescript-strict-plugin/issues/61.