Skip to content

Don't error with "used before assigned" when declared type includes void #52054

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

Conversation

Andarist
Copy link
Contributor

fixes #52003

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Dec 30, 2022
@@ -27313,7 +27317,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return convertAutoToAny(flowType);
}
}
else if (!assumeInitialized && !containsUndefinedType(type) && containsUndefinedType(flowType)) {
else if (!assumeInitialized && !containsUndefinedType(type) && !containsVoidType(type) && containsUndefinedType(flowType)) {
Copy link
Contributor Author

@Andarist Andarist Dec 30, 2022

Choose a reason for hiding this comment

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

The problem is that the typeof expression is the antecedent of the value variable here and it changes the void type somewhat unexpectedly.

narrowTypeByLiteralExpression discards void from string | void type for the assumeTrue === false case here.

So when we get to this line here we have:

type // string | void
flowType // string | undefined

As the other antecedent pushes undefined into the antecedentTypes in the getTypeAtFlowBranchLabel.

I don't think this can be changed since this is how TS behaves today:

declare const a: string | void

if (typeof a === 'undefined') {
  a // undefined
}

if (typeof a !== 'undefined') {
  a // string
}

So checking against voidType here was my best shot at fixing this.

EDIT: Alternatively... maybe the antecedents are set incorrectly here. I don't quite understand why value is "linked" to this binary expression. I didn't dive into CFA implementation too much yet so I didn't even attempt to explore the possibilities there

@fatcerberus
Copy link

FWIW:
While the “used before assigned” error definitely seems like a bug, I note that it’s never sound to narrow void out of a union, as the whole point of void is that it specifies a “return arity” of zero, and any function type with higher return arity (i.e. any other return type) is assignable to it. So if you do something like cond ? voidRet() : stringRet() and get a string | void, you can’t be sure, even if you get a string back, that it’s not bogus, since voidRet may in fact have returned a string, but its void return type is an implicit promise that the caller won’t access anything it happens to return (since in reality “zero return arity” isn’t a thing).

@Andarist
Copy link
Contributor Author

@fatcerberus ye, I realize that - but TS is very conservative about breaking changes, and apparently this is the current behavior:

declare const a: string | void

if (typeof a === 'undefined') {
  a // undefined
}

if (typeof a !== 'undefined') {
  a // string
}

That's why I've implemented this as a fix. Personally, I would prepare a different fix - one that wouldn't remove void from the union etc but to do that I would have to get a signal from the TS team member that this is what they want.

@sandersn
Copy link
Member

From reading #52003, I think the Not A Defect tag means that we don't want to fix that bug. So I think this PR should be closed?

@RyanCavanaugh feel free to speak up if I interpreted Not A Defect incorrectly.

@fatcerberus
Copy link

FWIW Not a Defect tag was added after the PR was opened.

@RyanCavanaugh
Copy link
Member

Yeah, this is the kind of fix we need to reject if we don't want to bleed performance forever. People should not be declaring variables of type void in the first place.

@Andarist Andarist deleted the fix/cfa-void-used-before-assigned branch January 12, 2023 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Unexpected error "Variable is used before being assigned."
5 participants