Code fixer improvements. #96

dragomirtitian · 2023-10-03T08:54:06Z

Code fixer improvements.

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

…d property as is if possible. Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

h-joo

LGTM with some nit and questions. Thank you for the changes!

tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports45-no-enum-members.ts

h-joo · 2023-10-06T09:49:18Z

tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports44-extract-other-to-variable.ts

+////    ]
+////}
+
+verify.codeFix({


The two fixes here seems to be identical

Not sure what you mean. The two fixes work in tandem here. The first will extract the property value to a variable and then the second one annotates the variable.

h-joo · 2023-10-06T10:02:13Z

tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports34-inline-import-default.ts

+// @declaration: true
+
+// @Filename: /person-code.ts
+////export type Person = { x: string; }


What happens if it's not exported? Is this worth a test case?

I'll investigate. I think it might be a TS error (using a private name). But I don't think we test for it.

h-joo · 2023-10-06T10:17:51Z

tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports29-fn-in-object-literal.ts

+// @declaration: true
+// fileName: code.ts
+////export const extensions = {
+////    /**


Nit: Do we need this comment?

I want to make sure the JS doc comment is preserved in the same position. With the previous way of adding the annotation (using replaceNode), comments were duplicated. I want to make sure we do not regress this.

h-joo · 2023-10-06T10:36:17Z

src/services/services.ts

@@ -510,7 +510,9 @@ function addSyntheticNodes(nodes: Node[], pos: number, end: number, parent: Node
                if (hasTabstop(parent)) {
                    continue;
                }
-                Debug.fail(`Did not expect ${Debug.formatSyntaxKind(parent.kind)} to have an Identifier in its trivia`);
+                // Not sure why this is here. When I try to make text change that contains only a property assigment this kicks in


Can you phrase this into a TODO?

h-joo · 2023-10-08T19:19:18Z

src/compiler/diagnosticMessages.json

@@ -6992,6 +6992,42 @@
        "category": "Message",
        "code": 90060
    },
+    "Add annotation of type '{0}'": {


These look great :)

I cannot find the usages, will these be standalone separate diagnostics, or do these fall under a sub-diagnostic i.e. Diagnostic.relatedInformation ?

They are used as the fixer descriptions in src\services\codefixes\fixMissingTypeAnnotationOnExports.ts.

external-declarations/src/code-mod/tsc-test-fixer/test-fixer.ts

external-declarations/src/test-runner/utils.ts

h-joo · 2023-10-08T20:04:41Z

external-declarations/src/code-mod/fixer/watch.ts

One question - do you intend to ship the interactive fixer in mainstream TS?

Probably not. A version of it will be in the tests probably, but I expect to reimplement it in a standalone project and open source that. But none of this has been set in stone, it's just my current thinking.

h-joo · 2023-10-08T20:13:17Z

src/compiler/checker.ts

-                if (propertySymbol.declarations) {
-                    const decl = first(propertySymbol.declarations);
+            const decl = propertySymbol.declarations && first(propertySymbol.declarations);
+


I don't understand what this code(the changes) is doing, can you explain with an example code snippet?

I wanted to be able to force the compiler to preserve the computed property expressions in types instead of resolving them, if the computed property is an accessible entity name (identifier, or identifier chain):

So for example for:

const A = 1; const E = { A: 2 } as const let o = { [A]: 1, [E.A]: 2 }

The checker would always produce as the type of o: { [1]: number, [2]: number } which is correct but not a type a dev would ever write. With this change, if A and E.A are accessible in the current scope (either are already in the scope or can be imported), they will be preserved and we get { [A]: number, [E.A]: number }

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

dragomirtitian requested a review from h-joo October 4, 2023 12:05

dragomirtitian added 14 commits October 4, 2023 13:57

Preserve formatting in the ID fixer.

570f403

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

Use imports instead of import types.

c6b60fc

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

Added more descriptive text for fixer messages and enabled fix all.

86d5751

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

Improve biding handling.

276f9ed

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

Ensure nested import types are processed.

9416f38

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

Added typeof fix for entity names.

24a5f53

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

Added support for relative typing of object literals with spread.

9c274fd

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

Fix spread on unique symbols. Add fix for array spread. Write compute…

60960f8

…d property as is if possible. Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

Added extract to variable refactor.

c474481

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

Remove new lines from code fix description.

192982e

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

Fix crash.

bee8fa8

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

Changed code mod to use language service.

c0b63bf

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

Fix code fixer issues for enums and for cases where the target changes.

62fd7be

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

Improve inline assertions

57d243a

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

dragomirtitian force-pushed the isolated-declarations-fixer branch from 90c8e5c to 57d243a Compare October 4, 2023 13:00

h-joo approved these changes Oct 9, 2023

View reviewed changes

dragomirtitian added 4 commits October 10, 2023 10:35

Removed fixed tests.

0dfdee1

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

Do not print typeof x for static readonly fields.

9b2fb26

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

Fix code review issues.

363e9d1

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

Fixed diagnostic message typo.

430bedc

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>

dragomirtitian force-pushed the isolated-declarations-fixer branch from 1cad6d7 to 430bedc Compare October 10, 2023 09:36

dragomirtitian merged commit 035429c into isolated-declarations Oct 10, 2023
1 check passed

dragomirtitian deleted the isolated-declarations-fixer branch November 17, 2023 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code fixer improvements. #96

Code fixer improvements. #96

dragomirtitian commented Oct 3, 2023

h-joo left a comment

h-joo Oct 6, 2023

dragomirtitian Oct 9, 2023

h-joo Oct 6, 2023

dragomirtitian Oct 9, 2023

h-joo Oct 6, 2023

dragomirtitian Oct 9, 2023

h-joo Oct 6, 2023

h-joo Oct 8, 2023

dragomirtitian Oct 9, 2023

h-joo Oct 8, 2023

dragomirtitian Oct 9, 2023

h-joo Oct 8, 2023

dragomirtitian Oct 9, 2023

Code fixer improvements. #96

Code fixer improvements. #96

Conversation

dragomirtitian commented Oct 3, 2023

h-joo 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

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