Add a code fixer for --isolatedDeclarations errors #58260

h-joo · 2024-04-19T22:57:41Z

Add a code-fixer to fix type errors for --isolatedDeclarations

This is built on top of the errors added in PR #58201.

This code fixer fixes the following patterns.
1. Adding missing types in places that type can be explicitly annotated.
2. Adding satisfies with type assertion on a value.
3. Create a namespace for expando functions and declare the property on the namespace.
4. Factor out expressions in heritage clauses (class B extends ), give them separate names and export them.

typescript-bot · 2024-04-19T22:57:46Z

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

typescript-bot · 2024-04-19T22:57:53Z

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

This code fixer fixes the following patterns. 1. Adding missing types on places that type can be explicitly annotated 2. Adding satisfies with type assertion on a value. 3. Create a namespace for expando functions and declare the property on the namespace. 4. Factor out expressions in heritage clauses (class B extends <expression>) and give them separate names. Signed-off-by: Hana Joo <hanajoo@google.com>

jakebailey · 2024-04-19T23:00:14Z

Seems like this may need a rebase now that the base PR is in.

h-joo · 2024-04-19T23:11:32Z

@h-joo please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
@microsoft-github-policy-service agree [company="{your company}"]
Options:

(default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
(when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"
Contributor License Agreement

@microsoft-github-policy-service agree company="Google"

src/services/codeFixProvider.ts

…Exports.ts

…ration is on.

jakebailey · 2024-04-22T18:12:58Z

I'm still going through the test cases, but one thing I'm not seeing are test cases for decorated classes/functions/etc. Even if that's not implemented yet, it may be worth adding some just to make sure they are covered.

jakebailey

Just finishing up reading the tests; I think overall I wonder if we need to be this complete, e.g. if it's worth handling cases like mixins, destructurings, and so on, which have very complicated conversions.

tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports7.ts

jakebailey · 2024-04-22T20:29:41Z

tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports8.ts

+    index: 0,
+    newFileContent:
+`function foo() {return 42;}
+export const g = function (): number { return foo(); };`,


I don't think we've added support for function expressions in initializers, right? So this isn't yet valid?

So we did add support for these AFAIK, this isn't an error under --isolatedDeclarations (I test it and it confirms that it isn't) . It errors by first looking at the variable, if there's no type annotation - goes into the initializer and sees if it's a function type and check all places have appropriate type annotation.

Hm, I guess I was going off of the list of unfinished things including initializers with functions...

tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports14.ts

tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports16.ts

jakebailey · 2024-04-22T21:49:32Z

tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports17-unique-symbol.ts

+// @isolatedDeclarations: true
+// @declaration: true
+// @lib: es2019
+//// export const a = Symbol();


At some point we really have to figure out how to make symbol inference better; I think we're blocked by the freshness problem I failed to resolve.

tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports21-params-and-return.ts

tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports23-heritage-formatting.ts

tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports30-inline-import.ts

tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports32-inline-short-hand.ts

tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports36-conditional-releative.ts

MichaelMitchell-at · 2024-04-23T08:12:03Z

Is it expected for this PR to be sufficient to work with Quick Fixes in VSCode, or with ts-fix? I've tried both with this branch and was not able to see any fix suggestions despite successfully getting isolatedDeclarations errors.

h-joo · 2024-04-23T14:39:54Z

Is it expected for this PR to be sufficient to work with Quick Fixes in VSCode, or with ts-fix? I've tried both with this branch and was not able to see any fix suggestions despite successfully getting isolatedDeclarations errors.

Good point, I tested this once with a locally built tsd, but I can't reproduce the result with the current state. We intend to support this in vscode as a quick fix and anywhere that uses tsd. Thank you for bringing this up, I'll try to figure out why this isn't working and fix it before merge.

jakebailey · 2024-04-23T14:58:03Z

Theoretically ts-fix should "just work", so that's definitely odd.

h-joo · 2024-04-23T15:17:06Z

Is it expected for this PR to be sufficient to work with Quick Fixes in VSCode, or with ts-fix? I've tried both with this branch and was not able to see any fix suggestions despite successfully getting isolatedDeclarations errors.

Actually, I'm seeing the code fixes. I'm doing development on a cloud machine so I'm using a browser to access vscode in my cloud machine, but it seems to work when I'm running this locally (through a remote desktop in the cloud machine). I don't understand why the difference 🤷🏻 , but it at least seems to work.

From the web interface (non-local)
From a local vscode run (remote-desktop)

src/services/codeFixProvider.ts

such as "A"|"B" --> string in case the d.ts emitter widens it. Also expose 'getWidenedLiteralType' as an internal API for the fixer to be able to imitate what the type checker is doing.

h-joo · 2024-04-24T15:55:27Z

I'm still going through the test cases, but one thing I'm not seeing are test cases for decorated classes/functions/etc. Even if that's not implemented yet, it may be worth adding some just to make sure they are covered.

Added test cases for experimental / non-experimental decorators, I think nothing should be affected as the decorators shoudn't change the types. It seems to work well from what I can see.

…expressions Widening of types only work on types from variable declaration, so when getWidenedLiteralType is being called on expressions the compiler wouldn't widen it. So call getWidenedLiteralType on the type of variables instead on those cases.

src/compiler/checker.ts

jakebailey

On the whole, this seems fine to me; I think we can get feedback on this early and see if people find the big literal ones "too much" or not. But, I'm guessing we won't end up with much feedback until the actual release.

src/services/codefixes/fixMissingTypeAnnotationOnExports.ts

andrewbranch

Couple of nits, overall I think the implementation looks really good.

src/services/codefixes/fixMissingTypeAnnotationOnExports.ts

andrewbranch · 2024-04-29T23:42:46Z

src/services/codefixes/fixMissingTypeAnnotationOnExports.ts

+    }
+    interface InferenceResult {
+        typeNode?: TypeNode | undefined;
+        mutatedTarget: boolean;


This looks spooky to me 😅

I'm assuming you're talking about the naming or would it be the logic?

I mean the strategy of having functions that conditionally mutate something and have to communicate whether they've done so... I didn't study it enough to fully understand why that was needed. Not necessarily a blocker, just a little eyebrow-raising

MichaelMitchell-at · 2024-04-30T06:34:36Z

Some feedback after trying the code fixer out on part of our codebase.

Unexpected:

Comments on object properties are duplicated in their respective types. This also seems to be a bug with the pre-existing "Infer function return type" editor refactor. When declaration files are emitted with inferred types, only /** docstring comments */ get copied (demo: https://tsplay.dev/m3dy1W (click ".D.TS" tab)), so I think the quickfix should be consistent with that, otherwise these duplicative comments just create noise.
```
export const x: {
    // this will be copied
    a: number;
} = {
    // this will be copied
    a: 42,
} satisfies {a: number};
```

Bug:

Quick fix isn't available for export const x = <div aria-label="hello" /> (under "jsx": "react-jsx") and ts-fix just crashes. It seems to be a problem parsing the - in aria-label. The crash happens here:

TypeScript/src/services/formatting/formatting.ts

Line 880 in c763c27

Debug.assert(tokenInfo.token.end === child.end, "Token end is child end");
ts-fix will sometimes insert types in the wrong places and produce syntactically invalid files. On the same files, using the "Add all missing type annotations" autofix does not have this problem. Submitting a reproduction will take some time, but the only commonality I've seen so far is that these types are complex and composed of multiple imported types, e.g import('some_module').Foo<{x: import('some_module').Bar, ...}>. Edit: I think this happens when there are multiple fixes in the same file that each add overlapping new imports. I was able to get around this issue by patching ts-fix to only apply one auto-fix to a file at a time. Much slower, but gets the job done.

jakebailey · 2024-04-30T15:06:42Z

Yeah, ts-fix itself has some problems that extend past just trying to use it with this PR; if you can figure out a fix that would of course be awesome to take over there, but it's definitely not a finished codebase.

src/services/codeFixProvider.ts

and reuse it. Remove suggestVariableName as this adds very little value compared to the code size it had.

src/compiler/diagnosticMessages.json

Co-authored-by: Andrew Branch <andrewbranch@users.noreply.github.com>

…de fixes for them

sheetalkamat · 2024-04-30T18:15:39Z

src/services/codeFixProvider.ts

        ...program.getSemanticDiagnostics(sourceFile, cancellationToken),
        ...program.getSyntacticDiagnostics(sourceFile, cancellationToken),
        ...computeSuggestionDiagnostics(sourceFile, program, cancellationToken),
    ];
+    if (getEmitDeclarations(program.getCompilerOptions())) {


If currently we are supporting providing code fix for errors only when --isolatedDeclarations is turned on may be should get declaration diagnostics only if its on to avoid perf hit

I had them change to this in #58260 (comment), though; in the editor, we'll have already called this for diags, and it's cached.

We can flip it back to just isolatedDeclarations if you think it'll be a problem, but it seemed less error prone.

Quoting Jake from the other comment

Actually, now that I think about it, this should probably just be getEmitDeclarations(program.getCompilerOptions()). It's possible that we could have quick fixes for declaration errors that are not in isolated declarations. And this call is cached so should be available for free anyway in codebases with declaration mode enabled.

Would you agree?

Oh, my comment was slightly late 😅

The cache does help a little but not whole lot since every edit will have to throw out the errors and recalculate them

But, we call this function already and offer them up as diagnostics in the editor; won't the cost already have been paid?

Yes i forgot about the part where LS.getSemanticDiagnostics gets declaration diagnostics as well.

Yeah, for posterity:

function getSemanticDiagnostics(fileName: string): Diagnostic[] { synchronizeHostData(); const targetSourceFile = getValidSourceFile(fileName); // Only perform the action per file regardless of '-out' flag as LanguageServiceHost is expected to call this function per file. // Therefore only get diagnostics for given file. const semanticDiagnostics = program.getSemanticDiagnostics(targetSourceFile, cancellationToken); if (!getEmitDeclarations(program.getCompilerOptions())) { return semanticDiagnostics.slice(); } // If '-d' is enabled, check for emitter error. One example of emitter error is export class implements non-export interface const declarationDiagnostics = program.getDeclarationDiagnostics(targetSourceFile, cancellationToken); return [...semanticDiagnostics, ...declarationDiagnostics]; }

jakebailey · 2024-04-30T20:01:50Z

I think all that's left for me is to just do the final format of the PR (one out of place import). Any other concerns?

MichaelMitchell-at · 2024-04-30T22:33:06Z

Just FYI, the aria-label case I mentioned above isn't ts-fix-specific. It just manifests in a worse way with ts-fix.

typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Apr 19, 2024

h-joo force-pushed the isolated-declaration-fixer branch from 12869dd to 4e506f9 Compare April 19, 2024 23:06

jakebailey reviewed Apr 19, 2024

View reviewed changes

src/services/codeFixProvider.ts Outdated Show resolved Hide resolved

h-joo added 2 commits April 22, 2024 08:07

(Non-functional) Apply 'npm run format' on fixMissingTypeAnnotationOn…

0191a46

…Exports.ts

Only get code fixes from declaration diagnostics when --isolatedDecla…

c9698a3

…ration is on.

h-joo force-pushed the isolated-declaration-fixer branch from 709e3fc to c9698a3 Compare April 22, 2024 08:40

jakebailey requested review from andrewbranch, jakebailey and iisaduan April 22, 2024 19:23

jakebailey reviewed Apr 22, 2024

View reviewed changes

MichaelMitchell-at reviewed Apr 23, 2024

View reviewed changes

src/services/codeFixProvider.ts Show resolved Hide resolved

h-joo added 2 commits April 24, 2024 13:16

Add test cases for experimental / non-experimental decorators

33e82b3

Suggest a type annotation for widened unions of type literals

5704f4e

such as "A"|"B" --> string in case the d.ts emitter widens it. Also expose 'getWidenedLiteralType' as an internal API for the fixer to be able to imitate what the type checker is doing.

h-joo force-pushed the isolated-declaration-fixer branch from be5529d to cb45ffe Compare April 26, 2024 17:23

npm run format

00e507c

jakebailey reviewed Apr 29, 2024

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

jakebailey approved these changes Apr 29, 2024

View reviewed changes

src/services/codefixes/fixMissingTypeAnnotationOnExports.ts Outdated Show resolved Hide resolved

src/services/codefixes/fixMissingTypeAnnotationOnExports.ts Outdated Show resolved Hide resolved

src/services/codefixes/fixMissingTypeAnnotationOnExports.ts Outdated Show resolved Hide resolved

andrewbranch reviewed Apr 29, 2024

View reviewed changes

MichaelMitchell-at mentioned this pull request Apr 30, 2024

Suggest autofixes for declaration diagnostics microsoft/ts-fix#30

Open

Take into account stylish changes from PR comments

023d32f

jakebailey reviewed Apr 30, 2024

View reviewed changes

src/services/codeFixProvider.ts Outdated Show resolved Hide resolved

Factor out a functionality to generate a variable name

38f6a3d

and reuse it. Remove suggestVariableName as this adds very little value compared to the code size it had.

andrewbranch reviewed Apr 30, 2024

View reviewed changes

src/compiler/diagnosticMessages.json Outdated Show resolved Hide resolved

h-joo and others added 5 commits April 30, 2024 19:05

Update src/compiler/diagnosticMessages.json

2086cb9

Co-authored-by: Andrew Branch <andrewbranch@users.noreply.github.com>

Update tests

03cc990

Remove unused imports, also use getEmitDeclarations to check to provi…

c4f0b81

…de fixes for them

Merge branch 'main' into isolated-declaration-fixer

69d0044

Fix the mis-resolution of merge conflict

48ed32b

sheetalkamat reviewed Apr 30, 2024

View reviewed changes

npm run format

1632489

jakebailey approved these changes Apr 30, 2024

View reviewed changes

jakebailey merged commit 33b1561 into microsoft:main Apr 30, 2024
28 checks passed

MichaelMitchell-at mentioned this pull request May 2, 2024

[isolatedDeclarations][5.5] Autofix does not work when JSX prop contains a dash - #58399

Open

ssalbdivad mentioned this pull request May 2, 2024

Migrate from explicit-boundary-types lint rule to --isolatedDeclarations arktypeio/arktype#943

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a code fixer for --isolatedDeclarations errors #58260

Add a code fixer for --isolatedDeclarations errors #58260

h-joo commented Apr 19, 2024

typescript-bot commented Apr 19, 2024

typescript-bot commented Apr 19, 2024

jakebailey commented Apr 19, 2024

h-joo commented Apr 19, 2024

jakebailey commented Apr 22, 2024

jakebailey left a comment

jakebailey Apr 22, 2024

h-joo Apr 23, 2024

jakebailey Apr 23, 2024

jakebailey Apr 22, 2024

MichaelMitchell-at commented Apr 23, 2024 •

edited

h-joo commented Apr 23, 2024 •

edited

jakebailey commented Apr 23, 2024

h-joo commented Apr 23, 2024 •

edited

h-joo commented Apr 24, 2024

jakebailey left a comment

andrewbranch left a comment

andrewbranch Apr 29, 2024

h-joo Apr 30, 2024

andrewbranch Apr 30, 2024

MichaelMitchell-at commented Apr 30, 2024 •

edited

jakebailey commented Apr 30, 2024

sheetalkamat Apr 30, 2024

jakebailey Apr 30, 2024

jakebailey Apr 30, 2024

h-joo Apr 30, 2024

h-joo Apr 30, 2024

sheetalkamat Apr 30, 2024

jakebailey Apr 30, 2024

sheetalkamat Apr 30, 2024

jakebailey Apr 30, 2024

jakebailey commented Apr 30, 2024

MichaelMitchell-at commented Apr 30, 2024

Add a code fixer for --isolatedDeclarations errors #58260

Add a code fixer for --isolatedDeclarations errors #58260

Conversation

h-joo commented Apr 19, 2024

typescript-bot commented Apr 19, 2024

typescript-bot commented Apr 19, 2024

jakebailey commented Apr 19, 2024

h-joo commented Apr 19, 2024

jakebailey commented Apr 22, 2024

jakebailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelMitchell-at commented Apr 23, 2024 • edited

h-joo commented Apr 23, 2024 • edited

jakebailey commented Apr 23, 2024

h-joo commented Apr 23, 2024 • edited

h-joo commented Apr 24, 2024

jakebailey left a comment

Choose a reason for hiding this comment

andrewbranch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelMitchell-at commented Apr 30, 2024 • edited

jakebailey commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakebailey commented Apr 30, 2024

MichaelMitchell-at commented Apr 30, 2024

MichaelMitchell-at commented Apr 23, 2024 •

edited

h-joo commented Apr 23, 2024 •

edited

h-joo commented Apr 23, 2024 •

edited

MichaelMitchell-at commented Apr 30, 2024 •

edited