-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Unwrap NoInfer
types when narrowing
#58292
Conversation
src/compiler/checker.ts
Outdated
declaredType = isNoInferType(declaredType) ? (declaredType as SubstitutionType).baseType : declaredType; | ||
initialType = isNoInferType(initialType) ? (initialType as SubstitutionType).baseType : initialType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first instinct was that I should re-wrap the result of narrowing with NoInfer
but quoting the original NoInfer
PR:
Other than blocking inference, NoInfer markers have no effect on T. Indeed, T and NoInfer are considered identical types in all other contexts.
So I don't see a reason why that would be necessary. I also tested something like this (TS playground) to see if this would be needed and I don't think it is:
declare function inner<T>(arg: T): T;
declare function outer<T>(a: T, cb: (arg: NoInfer<T>) => void): void;
outer({ foo: "bar" }, (arg) => {
arg
// ^? (parameter) arg: NoInfer<{ foo: string; }>
const result = inner(arg); // inferred despite `arg` being a `NoInfer` type!
// ^? const result: NoInfer<{ foo: string; }>
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's fine to erase NoInfer
in variable, parameter, and property references, but I think it would be simpler and more consistent to do it first thing in the getNarrowableTypeForReference
function. I think that's all you'd need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved it to getNarrowableTypeForReference
, works like a charm - thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above.
@sandersn @RyanCavanaugh could this one land before the upcoming RC? |
8e672ba
to
34a6d3d
Compare
* upstream/main: (37 commits) Added NoTruncation flag to completions (microsoft#58719) Clone node to remove location even when it has been modified if needed (microsoft#58706) Properly account for `this` argument in intersection apparent type caching (microsoft#58677) Fix: Include Values of Script Extensions for Unicode Property Value Expressions in Regular Expressions (microsoft#58615) In `reScanSlashToken` use `charCodeChecked` not `codePointChecked` (microsoft#58727) Shorten error spans for errors reported on constructor declarations (microsoft#58061) Mark file as skips typechecking if it contains ts-nocheck (microsoft#58593) Fixed an issue with broken `await using` declarations in `for of` loops (microsoft#56466) Do not expand type references in keyof and index access (microsoft#58715) Improve the performance of isolatedDeclarations quickfix (microsoft#58722) Unwrap `NoInfer` types when narrowing (microsoft#58292) Recover from type reuse errors by falling back to inferred type printing (microsoft#58720) Fixing self import (microsoft#58718) Enable JS emit for noCheck and noCheck for transpileModule (microsoft#58364) Revert PR 55371 (microsoft#58702) Update dependencies (microsoft#58639) Fix baselines after PR 58621 (microsoft#58705) Do not infer `yield*` type from contextual `TReturn` (microsoft#58621) `await using` normative changes (microsoft#58624) Handling statements from a known source file (microsoft#58679) ...
fixes #58266