Display property types in optional chains without optional type marker #53804

Andarist · 2023-04-16T21:29:45Z

Andarist · 2023-04-16T21:31:48Z

src/compiler/checker.ts

@@ -27526,7 +27526,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
                location = location.parent;
            }
            if (isExpressionNode(location) && (!isAssignmentTarget(location) || isWriteAccess(location))) {
-                const type = getTypeOfExpression(location as Expression);
+                const type = location.flags & NodeFlags.OptionalChain ?
+                    checkPropertyAccessChain(location as PropertyAccessChain, CheckMode.Normal, /*wasOptional*/ false) :


Note that this might not contain some guard rails related to control flow analysis that are present in getTypeOfExpression. I'm not sure if they are relevant for this case though.

Wouldn't this work as well?

const type = removeOptionalTypeMarker(getTypeOfExpression(location as Expression));

For an expression like a?.b.c, hovering over c would still show C | undefined (since its the end of the chain and the optional type marker is replaced with undefined), but hovering over b would just show B.

The upside is that getTypeOfExpression caches, so you avoid recomputing a type that's already cached.

yeah, that works fine I think - I pushed out the requested change

There's no need to make this conditional on location.flags & NodeFlags.OptionalChain. The marker type will only be present for optional chains.

cool, removed this redundant check 👍

rbuckton · 2023-04-27T21:31:49Z

src/compiler/checker.ts

@@ -27526,7 +27526,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
                location = location.parent;
            }
            if (isExpressionNode(location) && (!isAssignmentTarget(location) || isWriteAccess(location))) {
-                const type = getTypeOfExpression(location as Expression);
+                const type = location.flags & NodeFlags.OptionalChain ?
+                    checkPropertyAccessChain(location as PropertyAccessChain, CheckMode.Normal, /*wasOptional*/ false) :


There's no need to make this conditional on location.flags & NodeFlags.OptionalChain. The marker type will only be present for optional chains.

Andarist · 2023-05-10T13:31:49Z

@sandersn hmm, it feels that the automation didn't move this to the "waiting on reviewers" column

sandersn · 2023-05-11T21:16:39Z

@Andarist yep, that part isn't automated yet

Andarist · 2023-06-09T16:56:19Z

@rbuckton @gabritto friendly 🏓

jakebailey · 2023-06-09T17:32:23Z

tests/cases/fourslash/quickInfoInOptionalChain.ts

+
+verify.quickInfoAt("1", "(property) A.arr: string[]");
+verify.quickInfoAt("2", "(property) Foo.bar: {\n    baz: string;\n}");
+verify.quickInfoAt("3", "(property) baz: string | undefined");


I'm confused as to the end result here; both bar and baz themselves are not declared as potentially undefined, and yet bar is not | undefined while baz is. Why should they behave differently?

I admit this is a little bit confusing and I'm open for feedback how to improve this further.

baz has | undefined because it's the end of the optional chain expression and the whole optional chain expression has | undefined. Without this bit you'd look at an if statement that would always~ be truthy. That | undefined is here to inform you that the whole expression can indeed result in both branches to be taken.

OTOH, bar doesn't have it because with it it looks really weird that we can just chain off this expression (foo?.bar) without guarding the .baz property access

I guess that makes sense, though I'd probably personally defer to @DanielRosenwasser 😄

I think that the intuition described is fine, but this example kind of shows some inconsistencies in this approach where method return types don't exhibit the same behavior.

Whether that is enough of a blocker, I don't know.

That's a good catch. I think that it's worth unifying those - I'd expect x?.method; to include undefined based on the given described intuition. I see how this is quite subtle though so perhaps not everybody would agree with me on this one and that's fine.

I think that this find doesn't have to block this PR, I can just prepare a follow up fixing it at some later point in time. Up to you though.

rbuckton

This looks good, though I'll give @jakebailey time to reply with any additional feedback before merging.

…in-display-nonnullable-type

DanielRosenwasser · 2023-06-13T17:10:21Z

@typescript-bot pack this

typescript-bot · 2023-06-13T17:10:25Z

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

typescript-bot · 2023-06-13T17:12:09Z

Hey @DanielRosenwasser, 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/155473/artifacts?artifactName=tgz&fileId=6BF06C37B292DD6D02F58922E5CF285D242DCFAE403BD8E407538BD961555EBF02&fileName=/typescript-5.2.0-insiders.20230613.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/[email protected]".;

DanielRosenwasser · 2023-06-15T18:11:09Z

src/compiler/checker.ts

@@ -27609,7 +27609,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
                location = location.parent;
            }
            if (isExpressionNode(location) && (!isAssignmentTarget(location) || isWriteAccess(location))) {
-                const type = getTypeOfExpression(location as Expression);
+                const type = removeOptionalTypeMarker(getTypeOfExpression(location as Expression));


I'm not that familiar with this part of the code. Why doesn't this also apply to the entirety of a?.b.c?

The "optional type" marker is not added at the outermost PropertyAccessExpression, only for inner optional chain parts. At the outermost PropertyAccessExpression, we just union it with the normal undefined type.

See propagateOptionalTypeMarker in checker.ts for the logic related to when the marker type is added vs a regular undefined.

rbuckton · 2023-06-23T00:25:14Z

@DanielRosenwasser, @jakebailey any further feedback? If not, I will merge this.

DanielRosenwasser · 2023-06-23T00:40:09Z

I agree that my point doesn't need to hold back the PR

ghost · 2023-06-30T15:12:33Z

I believe this is still incorrect. Here's an extended example from the original issue:

interface A {
  arr: string[];
}

function test(a?: A): string {
  return a?.arr.length ? "A" : "B"; // Hovering `length` shows "(property) Array<String>.length: number | undefined"
}

function test2(a: string[] | undefined) {
  return a?.length // Hovering `length` shows "(property) Array<String>.length: number | undefined"
}

function test3(a: string[]) {
  return a.length // Hovering `length` shows "(property) Array<String>.length: number"
}

This feels wrong, because Array<String>.length can still only be number (as in test3).
The tooltip also shows the explanation for the length function, but for test and test2 one can get undefined which isn't explained in the description for length.
Again, the origin of undefined is not the property, but the expression it's used in.

Alternatively, an option to display it would be along the lines of (property) undefined | Array<String>.length: number (which shows that it's either undefined before reaching Array<String>).
I think this could also reveal potential ambiguity for undefined like (property) undefined | CustomClass.customFn: number | undefined.
Some other variation instead of the undefined | prefix could also be possible.

Edit: I assume this is what https://github.com/microsoft/TypeScript/pull/53804/files#r1224576390 was about.

Display property types in optional chains without optional type marker

4d5241e

typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Apr 16, 2023

Andarist commented Apr 16, 2023

View reviewed changes

sandersn assigned gabritto Apr 25, 2023

sandersn requested review from rbuckton and gabritto April 25, 2023 18:08

Tweak the implementation as suggested in the review

81934f5

rbuckton suggested changes Apr 27, 2023

View reviewed changes

remove redundant check

227bb9b

Andarist requested a review from rbuckton April 27, 2023 22:12

jakebailey reviewed Jun 9, 2023

View reviewed changes

rbuckton approved these changes Jun 9, 2023

View reviewed changes

rbuckton requested a review from DanielRosenwasser June 12, 2023 18:50

Merge remote-tracking branch 'origin/main' into props-in-optional-cha…

d107980

…in-display-nonnullable-type

DanielRosenwasser reviewed Jun 15, 2023

View reviewed changes

jakebailey approved these changes Jun 23, 2023

View reviewed changes

rbuckton merged commit 39da6b1 into microsoft:main Jun 23, 2023

Andarist mentioned this pull request Aug 14, 2024

Potential Optional Chaining Bug #41585

Closed

Display property types in optional chains without optional type marker #53804

Display property types in optional chains without optional type marker #53804

Uh oh!

Conversation

Andarist commented Apr 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andarist commented May 10, 2023

Uh oh!

sandersn commented May 11, 2023

Uh oh!

Andarist commented Jun 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbuckton left a comment

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser commented Jun 13, 2023

Uh oh!

typescript-bot commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbuckton commented Jun 23, 2023

Uh oh!

DanielRosenwasser commented Jun 23, 2023

Uh oh!

ghost commented Jun 30, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

typescript-bot commented Jun 13, 2023 •

edited

Loading

typescript-bot commented Jun 13, 2023 •

edited

Loading

ghost commented Jun 30, 2023 •

edited by ghost

Loading