Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 23, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4d5241e
Display property types in optional chains without optional type marker
Andarist Apr 16, 2023
81934f5
Tweak the implementation as suggested in the review
Andarist Apr 26, 2023
227bb9b
remove redundant check
Andarist Apr 27, 2023
d107980
Merge remote-tracking branch 'origin/main' into props-in-optional-cha…
Andarist Jun 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (getExportSymbolOfValueSymbolIfExported(getNodeLinks(location).resolvedSymbol) === symbol) {
return type;
}
Expand Down
28 changes: 28 additions & 0 deletions tests/cases/fourslash/quickInfoInOptionalChain.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/// <reference path='fourslash.ts'/>
//
// @strict: true
//
//// interface A {
//// arr: string[];
//// }
////
//// function test(a?: A): string {
//// return a?.ar/*1*/r.length ? "A" : "B";
//// }
////
//// interface Foo { bar: { baz: string } };
//// declare const foo: Foo | undefined;
////
//// if (foo?.b/*2*/ar.b/*3*/az) {}
////
//// interface Foo2 { bar?: { baz: { qwe: string } } };
//// declare const foo2: Foo2;
////
//// if (foo2.b/*4*/ar?.b/*5*/az.q/*6*/we) {}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

verify.quickInfoAt("4", "(property) Foo2.bar?: {\n baz: {\n qwe: string;\n };\n} | undefined");
verify.quickInfoAt("5", "(property) baz: {\n qwe: string;\n}");
verify.quickInfoAt("6", "(property) qwe: string | undefined");