Fix unassignable properties by adding undefined with exactOptionalPropertyTypes #45032

sandersn · 2021-07-14T22:23:30Z

Provide a specific error and codefix for missing undefined in target types with exactOptionalPropertyTypes: true.

The codefix skips paths starting with lib. and containing node_modules
Codefixes cannot use an elaboration as a trigger code. And the codefix logically needs to fix all of the properties in the target at once. So the resulting error message is a little vague. It still relies on reading the elaboration to find out the first erroneous property.

Doesn't cover or test any complicated variations.

Destructuring does not. But - skipping node_modules and lib.* does. - call expressions does - property access, including with private identifiers, does

As long as it's not nested

sandersn · 2021-07-14T22:24:33Z

@iisaduan you might be interested in using this in your codefix tool when it's ready.

src/services/codefixes/addOptionalPropertyUndefined.ts

andrewbranch · 2021-07-22T20:44:58Z

@typescript-bot pack this

typescript-bot · 2021-07-22T20:45:01Z

Heya @andrewbranch, I've started to run the tarball bundle task on this PR at d058191. You can monitor the build here.

typescript-bot · 2021-07-22T21:00:29Z

Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/107256/artifacts?artifactName=tgz&fileId=A249FC3C1F4E540B70977C44884C27EB0042FB333AB3BDDFA2F202C4AE39B06A02&fileName=/typescript-4.4.0-insiders.20210722.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.4.0-pr-45032-3".;

andrewbranch · 2021-07-22T23:37:08Z

I was surprised that neither of these cases are covered:

// @exactOptionalPropertyTypes: true

interface User {
    name: string;
    email?: string;
}

const user: User = {
    name: "Andrew",
    email: undefined,
};

user.email = undefined;

sandersn

There should be 4 comments below unless github lost two.

sandersn · 2021-08-03T16:09:51Z

tests/baselines/reference/strictOptionalProperties1.errors.txt

        obj.b = undefined;
    }

    function f2(obj: { a?: string, b?: string | undefined }) {
        obj = obj;
        obj.a = obj.a;  // Error
        ~~~~~
-!!! error TS2322: Type 'string | undefined' is not assignable to type 'string'.
-!!! error TS2322:   Type 'undefined' is not assignable to type 'string'.
+!!! error TS2412: Type 'string | undefined' is not assignable to type 'string' with exactOptionalPropertyTypes: true. Consider adding 'undefined' to the type of the target.


this error is not obvious, but self-assignments of optional properties require adding undefined

sandersn · 2021-08-03T16:11:24Z

tests/baselines/reference/strictOptionalProperties1.errors.txt

@@ -100,8 +111,8 @@ tests/cases/compiler/strictOptionalProperties1.ts(119,5): error TS2411: Property
    function f4a(t1: [number, string?], t2: [number, string?, string?]) {
        t1 = t2;  // Wasn't an error, but should be
        ~~
-!!! error TS2322: Type '[number, string?, string?]' is not assignable to type '[number, string?]'.
-!!! error TS2322:   Target allows only 2 element(s) but source may have more.
+!!! error TS2375: Type '[number, string?, string?]' is not assignable to type '[number, string?]' with exactOptionalPropertyTypes: true. Consider adding 'undefined' to the types of the target's properties.


This error needs two fixes, and now we suggest one in the elaboration and the other as a codefix.

This is confusing because they appear to disagree, but I don't know that it's worth fixing.

sandersn · 2021-08-03T16:12:20Z

src/compiler/checker.ts

@@ -16753,24 +16755,29 @@ namespace ts {
                let sourcePropType = getIndexedAccessTypeOrUndefined(source, nameType);
                if (!sourcePropType) continue;
                const propName = getPropertyNameFromIndex(nameType, /*accessNode*/ undefined);
-                const targetIsOptional = !!(propName && (getPropertyOfType(target, propName) || unknownSymbol).flags & SymbolFlags.Optional);


I moved this verbatim inside the new else

sandersn · 2021-08-03T16:13:21Z

src/compiler/checker.ts

-                        reportedError = true;
-                    }
-                    else {
+                    reportedError = true;


driveby fix; reportedError was always set to true in this block, regardless of whether elaborated is true, and it's not checked anywhere in this block, so it's fine to set it at the top.

sandersn · 2021-08-03T16:15:24Z

This is ready to review again; it was a surprising amount of work to add the two other cases that @andrewbranch pointed out.

andrewbranch

Not a blocker, but these new diagnostics seem like perfect candidates for related diagnostics of '{0}' was declared here. Presumably “here” would be the result of getSourceTarget, and your codefix could actually pull the related diagnostic location out instead of calculating it there.

src/compiler/checker.ts

src/services/codefixes/addOptionalPropertyUndefined.ts

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

sandersn · 2021-08-03T17:21:38Z

seem like perfect candidates for related diagnostics of '{0}' was declared here.

I'll take a look at how complicated that is. The first draft has some add-related-span code that runs afterward already. The two newer errors are separate from all the drill-down error code, though.

, and your codefix could actually pull the related diagnostic location out instead of calculating it there.

Codefixes only get an error code and a span, not the actual diagnostic. I could change that but I don't have the time right now.

sandersn · 2021-08-04T16:59:03Z

I went poking through other codefix's code and found getFixableErrorSpanExpression, so I moved that to services/utilities and started using it.

andrewbranch · 2021-08-04T17:01:13Z

@typescript-bot pack this

typescript-bot · 2021-08-04T17:01:16Z

Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 3ffe9db. You can monitor the build here.

typescript-bot · 2021-08-04T17:17:22Z

Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/108060/artifacts?artifactName=tgz&fileId=F0C3DB42A3E1CED41191A78EF461A657928E21C2AA18C1CD5BF4D0802C4308CC02&fileName=/typescript-4.4.0-insiders.20210804.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.4.0-pr-45032-20".;

andrewbranch · 2021-08-04T17:56:38Z

@sandersn it now crashes here: /~https://github.com/microsoft/playwright/blob/869f8d541b60fae388637e83243a642b0f225e4b/src/server/network.ts#L352

errorNode in your codefix is the ObjectLiteralExpression. i is 0. n.valueDeclaration is the PropertyDeclaration:

private _finishedPromiseCallback: (arg: { error?: string }) => void = () => {};

andrewbranch · 2021-08-04T19:45:52Z

I wrapped each individual request for codefixes in a try/catch and there was one other place in playwright that failed with the same crash. Otherwise, what became apparent is there are still a lot of assignability errors that this codefix doesn’t cover, because we started out with 253 errors (8 were due to me failing to run some codegen as part of the build, I think), and after applying this codefix across all files repeatedly until it was no longer offered (except for the two places that crashed), we actually went up to 404 errors. Essentially, the assignability errors just move (and apparently multiply) into places that we don’t identify as fixable.

Possible conclusions:

We should attempt to identify a lot more assignability errors as fixable with this codefix
Provide upgrade tooling for --strictOptionalProperties #44419 is probably not feasible for large/complex projects (which is where it would be valuable)

If you want to see what unfixable errors remain after running the tool, I’ve committed them to a fork (one commit per run, and each run actually does two passes of codefixes when it discovers overlapping changes): /~https://github.com/andrewbranch/playwright/tree/exactOptionalPropertyTypes

sandersn · 2021-08-04T23:23:47Z

Another possible conclusion:

We need more analysis of the new errors to figure out how the codefix causes the new errors. I thought that adding undefined wouldn't change anything since that used to be the semantics, but it sounds like I was wrong.

In the meantime, we need to decide between shipping a half-baked codefix or not. We've done this before! But for a new pedantic feature, I don't think we'll do much better than codemods written by the experts who adopt new features. So I'm leaning toward not shipping it.

@DanielRosenwasser do you have an opinion about this?

andrewbranch · 2021-08-05T01:37:48Z

I thought that adding undefined wouldn't change anything since that used to be the semantics, but it sounds like I was wrong.

I believe the deal is that if you naively add | undefined to every optional property, you wind up with no errors, minus the ones you couldn’t change because they were in node_modules etc. But the codefix doesn’t want to add | undefined to every optional property; it just does the ones that it identifies as needing to change. Here’s an example of a contrived program with one fixable error that turns into two unfixable ones upon being fixed.

// @exactOptionalPropertyTypes: true

interface A1 {
  a?: string | number;
}

interface A2 {
  a?: string | boolean;
}

interface B extends A1, A2 {
  a?: string;
}

declare let b: B;
b.a = undefined;
//^^ Type 'undefined' is not assignable to type 'string' with 'exactOptionalPropertyTypes: true'. Consider adding 'undefined' to the type of the target.

Fixing the property assignment adds an “incorrectly extends” error for each of B’s extended interfaces.

sandersn · 2021-08-05T13:21:25Z

I took a look at your branch and got 398 errors (after realising I incorrectly re-generated the types.d.ts after cloning the repo). 168 of those used the new errors, but I guess didn't offer a codefix.

I'm looking at the errors right now. I'll edit this comment as I survey them:

So far:

Codefix alters derived but not base types. Detected via an extends check, so the new error doesn't display.
Codefix alters a widely used type but not a structurally related subtype. Here, detected via an extends check, so the new error doesn't display.
Codefix can't modify a property through an intersection and a mapped type. (Not following map types seems reasonable in general, although both were Omit)

The very first error, in src/browserServerImpl.ts, was extremely confusing. As far as I can tell, CallMetadata has a lot of optional properties, and (at least) CallMetadata.error gets undefined assigned to it. So the codefix adds undefined to its properties. The type Metadata happens not to get any undefineds, and the codefix doesn't. Then CallMetadata gets assigned indirectly to Metadata | undefined via an extends check. The extends check is deep enough that the new error doesn't trigger.

export type CallMetadata = {
    params: any;
    apiName?: string | undefined;
    stack?: {}[] | undefined;
};
export type Metadata = {
    stack?: {}[],
    apiName?: string,
};
export interface BrowserChannel {
  newContext(metadata?: Metadata): void
}
class ConnectedBrowserDispatcher implements BrowserChannel {
    newContext(metadata: CallMetadata): void {}
}

Also, the error is very misleading, given the context. Metadata was previously assignable to CallMetadata, so undefined didn't matter. But the error now reports that undefined isn't assignable to CallMetadata, which made me think it implied that Metadata was still. Directly assigning clears things up:

declare var md: Metadata | undefined
declare var cmd: CallMetadata
md = cmd

Now the new error fires and the elaboration tells me that stack is at fault. But the codefix still isn't offered.

The same failure happened in src/client/accessibility.ts and this time the elaboration concluded that a parameter type was at fault, not the return type that was actually mismatched. It finally concluded, nonsensically, "ElementHandle<Node> is missing the following properties from ElementHandle<Node>: (followed by a list of 30+ properties)".
The same failure happened in src/android/android.ts, with the same type, and this time the error message was correct, but the new error was embedded in a pyramid message of doom 20 lines long.

sandersn · 2021-08-05T16:21:27Z

I think this is the wrong way to fix exactOptionalPropertyTypes errors. Applying the fix produces errors that (1) occur a long way from the true cause (2) penalises slightly-bad code like having structurally related types with no extends clause, or combining options types with a bunch of type arithmetic.

I think the better fix is to unconditionally add | undefined to all optional properties, and then have a human look at the diff and decide which places should not allow undefined.

The new error messages are often correct, so it might be worthwhile to leave them. However, I'm not sure their advice is clear enough without a codefix to back them up.

sandersn · 2021-08-05T17:18:49Z

src/services/codefixes/addOptionalPropertyUndefined.ts

+            return emptyArray;
+        }
+        const targetPropertyType = getTargetPropertyType(checker, targetNode);
+        if (targetPropertyType && checker.isExactOptionalPropertyMismatch(source, targetPropertyType)) {


maybe find parent type and fix all, like the other cases

andrewbranch · 2021-08-05T17:48:58Z

@typescript-bot pack this

typescript-bot · 2021-08-05T17:49:04Z

Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 3e70798. You can monitor the build here.

typescript-bot · 2021-08-05T18:06:10Z

Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/108150/artifacts?artifactName=tgz&fileId=7ADF1C6077E15A73364CB5E8915A1112B3FAF7CC07D162080EF309FB2D367F7202&fileName=/typescript-4.4.0-insiders.20210805.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.4.0-pr-45032-29".;

whether or not somebody tried to assign undefined to them in the erroneous assignment

sandersn · 2021-08-05T21:50:16Z

I think this is ready now. I simplified the fix to add | undefined to all properties in a type, regardless of whether the source could assign undefined to it, because that's more likely to result in a working build afterward.

And I got rid of the fix-all, since it's not a good idea. Thanks @amcasey for suggesting this.

andrewbranch · 2021-08-05T21:57:58Z

@typescript-bot pack this one last time please 🙃

typescript-bot · 2021-08-05T21:58:03Z

Heya @andrewbranch, I've started to run the tarball bundle task on this PR at d535509. You can monitor the build here.

typescript-bot · 2021-08-05T22:15:44Z

Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/108163/artifacts?artifactName=tgz&fileId=6D07C60947B8E7E066B6051E8EEC18E4F437CFEFB1B60AE1DE53F9B727A9876D02&fileName=/typescript-4.4.0-insiders.20210805.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.4.0-pr-45032-33".;

…pertyTypes (microsoft#45032) * Simple first version Doesn't cover or test any complicated variations. * Lots of cases work Destructuring does not. But - skipping node_modules and lib.* does. - call expressions does - property access, including with private identifiers, does * Support variable declarations, property assignments, destructuring As long as it's not nested * More cleanup * skip all d.ts, not just node_modules/lib * Offer a codefix for a lot more cases * remove incorrect tuple check * Use getSymbolId instead of converting to string Co-authored-by: Andrew Branch <andrewbranch@users.noreply.github.com> * add test + switch to tracking number symbol ids * Address PR comments * Exclude tuples from suggestion * Better way to get error node Plus add a check that errorNode is an argument to the call, not the call's expression. * fix semicolon lint * fix another crash * Simplify: add undefined to all optional propertie whether or not somebody tried to assign undefined to them in the erroneous assignment * remove fix-all Co-authored-by: Andrew Branch <andrewbranch@users.noreply.github.com>

sandersn added 5 commits July 13, 2021 08:36

Simple first version

664d749

Doesn't cover or test any complicated variations.

Merge branch 'main' into fix-exact-optional-unassignable-properties

d1008eb

Lots of cases work

671096b

Destructuring does not. But - skipping node_modules and lib.* does. - call expressions does - property access, including with private identifiers, does

Support variable declarations, property assignments, destructuring

a728852

As long as it's not nested

More cleanup

715f235

typescript-bot assigned sandersn Jul 14, 2021

typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Jul 14, 2021

sandersn requested review from DanielRosenwasser and gabritto July 14, 2021 22:23

DanielRosenwasser reviewed Jul 14, 2021

View reviewed changes

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

DanielRosenwasser mentioned this pull request Jul 14, 2021

Strict optional properties #43947

Merged

sandersn added 2 commits July 15, 2021 09:01

skip all d.ts, not just node_modules/lib

cfea6e1

Merge branch 'main' into fix-exact-optional-unassignable-properties

d058191

sandersn added 4 commits July 30, 2021 09:12

Merge branch 'main' into fix-exact-optional-unassignable-properties

334ca65

Offer a codefix for a lot more cases

41f0525

Merge branch 'main' into fix-exact-optional-unassignable-properties

09c916f

remove incorrect tuple check

cdd40d3

sandersn commented Aug 3, 2021

View reviewed changes

andrewbranch reviewed Aug 3, 2021

View reviewed changes

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

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

sandersn and others added 2 commits August 3, 2021 10:06

Use getSymbolId instead of converting to string

f018b8e

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

add test + switch to tracking number symbol ids

960e657

fix semicolon lint

3ffe9db

fix another crash

3e70798

sandersn commented Aug 5, 2021

View reviewed changes

sandersn added 2 commits August 5, 2021 13:43

Simplify: add undefined to all optional propertie

4b7b3fe

whether or not somebody tried to assign undefined to them in the erroneous assignment

remove fix-all

d535509

sandersn added 2 commits August 6, 2021 05:27

Merge branch 'main' into fix-exact-optional-unassignable-properties

e027d1e

Merge branch 'main' into fix-exact-optional-unassignable-properties

cc51d23

sandersn merged commit 8d4fe5a into main Aug 10, 2021

sandersn deleted the fix-exact-optional-unassignable-properties branch August 10, 2021 23:57

andrewbranch mentioned this pull request Aug 20, 2021

Provide upgrade tooling for --strictOptionalProperties #44419

Closed

peterblazejewicz mentioned this pull request Sep 30, 2021

[mdast] remove unused dslint ignores DefinitelyTyped/DefinitelyTyped#56064

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unassignable properties by adding undefined with exactOptionalPropertyTypes #45032

Fix unassignable properties by adding undefined with exactOptionalPropertyTypes #45032

sandersn commented Jul 14, 2021

sandersn commented Jul 14, 2021

andrewbranch commented Jul 22, 2021

typescript-bot commented Jul 22, 2021 •

edited

Loading

typescript-bot commented Jul 22, 2021 •

edited

Loading

andrewbranch commented Jul 22, 2021

sandersn left a comment

sandersn Aug 3, 2021

sandersn Aug 3, 2021

sandersn Aug 3, 2021

sandersn Aug 3, 2021

sandersn commented Aug 3, 2021

andrewbranch left a comment

sandersn commented Aug 3, 2021

sandersn commented Aug 4, 2021

andrewbranch commented Aug 4, 2021

typescript-bot commented Aug 4, 2021 •

edited

Loading

typescript-bot commented Aug 4, 2021 •

edited

Loading

andrewbranch commented Aug 4, 2021

andrewbranch commented Aug 4, 2021 •

edited

Loading

sandersn commented Aug 4, 2021

andrewbranch commented Aug 5, 2021 •

edited

Loading

sandersn commented Aug 5, 2021 •

edited

Loading

sandersn commented Aug 5, 2021

sandersn Aug 5, 2021

andrewbranch commented Aug 5, 2021

typescript-bot commented Aug 5, 2021 •

edited

Loading

typescript-bot commented Aug 5, 2021 •

edited

Loading

sandersn commented Aug 5, 2021

andrewbranch commented Aug 5, 2021

typescript-bot commented Aug 5, 2021 •

edited

Loading

typescript-bot commented Aug 5, 2021 •

edited

Loading

Fix unassignable properties by adding undefined with exactOptionalPropertyTypes #45032

Fix unassignable properties by adding undefined with exactOptionalPropertyTypes #45032

Conversation

sandersn commented Jul 14, 2021

sandersn commented Jul 14, 2021

andrewbranch commented Jul 22, 2021

typescript-bot commented Jul 22, 2021 • edited Loading

typescript-bot commented Jul 22, 2021 • edited Loading

andrewbranch commented Jul 22, 2021

sandersn left a comment

Choose a reason for hiding this comment

sandersn Aug 3, 2021

Choose a reason for hiding this comment

sandersn Aug 3, 2021

Choose a reason for hiding this comment

sandersn Aug 3, 2021

Choose a reason for hiding this comment

sandersn Aug 3, 2021

Choose a reason for hiding this comment

sandersn commented Aug 3, 2021

andrewbranch left a comment

Choose a reason for hiding this comment

sandersn commented Aug 3, 2021

sandersn commented Aug 4, 2021

andrewbranch commented Aug 4, 2021

typescript-bot commented Aug 4, 2021 • edited Loading

typescript-bot commented Aug 4, 2021 • edited Loading

andrewbranch commented Aug 4, 2021

andrewbranch commented Aug 4, 2021 • edited Loading

sandersn commented Aug 4, 2021

andrewbranch commented Aug 5, 2021 • edited Loading

sandersn commented Aug 5, 2021 • edited Loading

sandersn commented Aug 5, 2021

sandersn Aug 5, 2021

Choose a reason for hiding this comment

andrewbranch commented Aug 5, 2021

typescript-bot commented Aug 5, 2021 • edited Loading

typescript-bot commented Aug 5, 2021 • edited Loading

sandersn commented Aug 5, 2021

andrewbranch commented Aug 5, 2021

typescript-bot commented Aug 5, 2021 • edited Loading

typescript-bot commented Aug 5, 2021 • edited Loading

typescript-bot commented Jul 22, 2021 •

edited

Loading

typescript-bot commented Jul 22, 2021 •

edited

Loading

typescript-bot commented Aug 4, 2021 •

edited

Loading

typescript-bot commented Aug 4, 2021 •

edited

Loading

andrewbranch commented Aug 4, 2021 •

edited

Loading

andrewbranch commented Aug 5, 2021 •

edited

Loading

sandersn commented Aug 5, 2021 •

edited

Loading

typescript-bot commented Aug 5, 2021 •

edited

Loading

typescript-bot commented Aug 5, 2021 •

edited

Loading

typescript-bot commented Aug 5, 2021 •

edited

Loading

typescript-bot commented Aug 5, 2021 •

edited

Loading