Skip to content

Control flow analysis overriding an explicit type declaration when the type flows from optional chaining #50444

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.

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

Closed
Andarist opened this issue Aug 25, 2022 · 9 comments
Labels
Not a Defect This behavior is one of several equally-correct options

Comments

@Andarist
Copy link
Contributor

Bug Report

I'm not exactly sure if this is a bug or not - CFA rules are not well-documented. I find the present behavior to be quite annoying though

🔎 Search Terms

narrowing, control flow analysis, nullable, optional chaining

🕗 Version & Regression Information

// purposefully use `@types/*` here because otherwise I had problems with ATA 
import '@types/vscode-webview'

const vscode: ReturnType<typeof acquireVsCodeApi> | undefined = acquireVsCodeApi?.()
//    ^? WebviewApi<unknown> | undefined

vscode
// ^? WebviewApi<unknown>

// attempt to wrap in an IIFE
const vscode2: ReturnType<typeof acquireVsCodeApi> | undefined = (() => acquireVsCodeApi?.())();
//    ^? WebviewApi<unknown> | undefined

// no change though
vscode2
// ^? WebviewApi<unknown>

const vscode3: ReturnType<typeof acquireVsCodeApi> | undefined = acquireVsCodeApi ? acquireVsCodeApi() : undefined;
//    ^? WebviewApi<unknown> | undefined

// finally 🎉
vscode3
// ^? WebviewApi<unknown> | undefined

🙁 Actual behavior

Referencing a variable with the declared type that originates in an expression using optional chaining doesn't include | undefined in its type.

Note that acquireVsCodeApi is always defined according to the types.

🙂 Expected behavior

Even though acquireVsCodeApi is always defined I would expect my explicit declaration on the assigned variable to win for the purpose of CFA.

I have an app that is running as a standalone but it's also bundled as a VS Code extension. It's a hussle to include the VS Code types conditionally and it's much easier for us to just add those defs globally and refine this stuff at runtime when needed.

@MartinJohns
Copy link
Contributor

That the type of whatever you assign takes priority over the annotation is working as intended. However, I'm surprised why undefined is missing when you use the optional chaining operator.

@Andarist
Copy link
Contributor Author

However, I'm surprised why undefined is missing when you use the optional chaining operator.

This is probably related to the fact that acquireVsCodeApi is not nullable here. However, if that would be the expected result then I would expect the ternary expression to behave in the same way but apparently, it doesn't. Perhaps an optional chaining here is treated similarly to binary expressions. With acquireVsCodeApi() || undefined we also won't get undefine as part of the final type because we know that || undefined is an impossible case. But again - IMHO the same should happen with ternary expression.

That the type of whatever you assign takes priority over the annotation is working as intended

If that's the case then OK - although I find it a little bit weird. As I've written down a manual annotation - I don't want to rely on inference here.

@MartinJohns
Copy link
Contributor

MartinJohns commented Aug 25, 2022

If that's the case then OK - although I find it a little bit weird.

Assignment narrows, that's the major point. It would be weird to assign a string and then be unable to access the string members, just because a different type could be assigned as well.

You can use a type assertion instead of an annotation, although that has some drawbacks too. You actually want #47920.

@Andarist
Copy link
Contributor Author

I know that semantics of the satisfies operator are not yet set in stone but from what i recall one of the leading semantic there was that it would „assert” the type without changing it. In my case i actually would like to change the type - i’d like to widen it.

This use case rly is not that important to ke though. I was just buffled when ive spotted this behavior as i didnt know about it (usually it doesnt matter so ive never noticed it). Adding on top of that the inconsistent behavior of ternary expressions i’ve thought that it is an actual bug

@fatcerberus
Copy link

The idea is that

let x: number | string = "foo";
takesAString(x);

should not behave differently from

let x: number | string;
// maybe some other stuff here
x = "foo";
takesAString(x);  // no type check needed, assigned on previous line

@Andarist
Copy link
Contributor Author

I see them slightly differently - the middle step between the declaration and the call is IMHO important. With it, I can see that there is CFA applied after I set a boundary for the "input" (the manual annotation). It makes sense that TS takes new information into account - it makes less sense to me that it ignores my manual boundary. But overall, as mentioned - I don't intend to fight this, it's an acceptable situation for me, I just have been caught off guard here.

I don't understand though why a type reported at the declaration matches my declared type but yet it's narrowed down just one line below that declaration. If this is expected behavior then IMHO they both should display the same.

@MartinJohns
Copy link
Contributor

I don't understand though why a type reported at the declaration matches my declared type but yet it's narrowed down just one line below that declaration. If this is expected behavior then IMHO they both should display the same.

See the discussion in #50002.

@RyanCavanaugh RyanCavanaugh added the Not a Defect This behavior is one of several equally-correct options label Aug 26, 2022
@RyanCavanaugh
Copy link
Member

Hovering on a declaration name shows you the declared type. This is the type that doesn't have any CFA applied -- effectively it's telling you what the possible, all-worlds value of a value would be.

Initialization does perform narrowing. Not doing this opens you up to stuff that just looks broken as heck; this used to not happen and everyone hated it.

For a const, obviously the CFA type you get from the initializer is the only type you're going to see going forward. But that's still not the declared type, and the difference is potentially observable, e.g.

type SU = string | undefined;

// Forward reference is legal here and gets declared type
// Initialization of undefined is plainly legal by inspection -- `typeof a` is SU absent CFA info
const b: typeof a = undefined;

const a: SU = "hello";
a.charAt(0); // Should be legal by inspection - 'a' was clearly initialized

There are a bunch of invariants you might want here and they can't all be simultaneously satisfied.

@github-actions
Copy link

github-actions bot commented Jun 8, 2023

This issue has been marked as 'Not a Defect' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Not a Defect This behavior is one of several equally-correct options
Projects
None yet
Development

No branches or pull requests

4 participants