-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Prevent infinite recursion resolving nested conditional types with import types in them #32097
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
Conversation
src/compiler/checker.ts
Outdated
@@ -10564,6 +10565,7 @@ namespace ts { | |||
function getTypeFromConditionalTypeNode(node: ConditionalTypeNode): Type { | |||
const links = getNodeLinks(node); | |||
if (!links.resolvedType) { | |||
links.resolvedType = resolvingConditionalType; |
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.
Usually we use a combination of pushTypeResolution
and popTypeResolution
(although admittedly, TypeSystemPropertyName
has no entry for Node.resolvedType
, so either it'd need to be added or we'd need to be OK with this), this way the error case when entering and exiting the recursion is explicitly handled. It should probably also report a circularity error using reportCircularityError
, or error(symbol.valueDeclaration, Diagnostics._0_is_referenced_directly_or_indirectly_in_its_own_type_annotation, name);
directly (since we're inevitably going to cache results using this unformed "circular" type).
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.
And a repro for the test harness would be swell, since this is exactly the kind of thing that could randomly go away in another bugfix without a test case demonstrating how it can occur. (In fact, without an isolated repro, it's hard to even know if this is the right place to insert a break)
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.
Yeah, totally agree, but I just can’t make sense out of the repro provided :/
@AnyhowStep with this change, your library produces the error “Conditional type circularly references itself” in several places. Here are a few of them:
If you can make any sense of this, it would be incredibly helpful if you could figure out a smaller, more focused repro that we can add to our tests. Especially if you think that the circularity error is incorrect—I’m not sure how we’re going to figure out how to diagnose this without a smaller example. Clarification: the version you mentioned in the original issue produces that error—master does not. |
Actually, this is fairly troubling: git clone AnyhowStep/type-mapping && cd type-mapping
git checkout 1a999792f00fe25d2d3d1038d48b857a82b94219 # v1.1.2
# no errors
node ../TypeScript/built/local/tsc.js -p .
# Before this PR: Maximum call stack size exceeded
# With this PR: 12 circularity errors
node ../TypeScript/built/local/tsc.js dist/fluent-lib/index.d.ts Not hitting the max call stack size is nice and all, but the root of the problem seems to be a declaration emit bug 😐 |
I'll take a look at it when I get back home but this reminds me a lot of this other thing that happened to me recently, Basically, I was using that exact library and forgot to add an explicit return type annotation to the generic function.
After adding an explicit return type annotation, the emitted I looked at the arrayLike<F extends import("..").Mapper<unknown, any>>(f: F): import("..").FluentMapper<import("..").Mapper<unknown, ArrayLike<ReturnType<F>>> & import("..").ExpectedInput<ArrayLike<Extract<((F extends (name: string, mixed: infer T, ...args: any) => any ? unknown extends T ? F extends import("..").ExpectedInput<infer T> ? [T] : F extends import("..").MappableInput<infer T> ? [T] : [ReturnType<F>] : [T] : never) extends any ? (k: F extends (name: string, mixed: infer T, ...args: any) => any ? unknown extends T ? F extends import("..").ExpectedInput<infer T> ? [T] : F extends import("..").MappableInput<infer T> ? [T] : [ReturnType<F>] : [T] : never) => void : never) extends (k: infer I) => void ? I : never, [any]>[0]>> & import("..").MappableInput<ArrayLike<Extract<((F extends (name: string, mixed: infer T, ...args: any) => any ? unknown extends T ? F extends import("..").MappableInput<infer T> ? [T] : F extends import("..").ExpectedInput<infer T> ? [T] : [ReturnType<F>] : [T] : never) extends any ? (k: F extends (name: string, mixed: infer T, ...args: any) => any ? unknown extends T ? F extends import("..").MappableInput<infer T> ? [T] : F extends import("..").ExpectedInput<
/*snip*/ Looks like If we look at the export function arrayLike<F extends AnySafeMapper> (f : F) {
return fluentMapper(m.arrayLike<F>(f));
} We can see that I forgot to supply an explicit return type annotation. So, In a newer version of the library, I've added explicit return type annotations, export function arrayLike<F extends AnySafeMapper> (f : F) : (
FluentMapper<m.ArrayLikeMapper<F>>
) {
return fluentMapper(m.arrayLike<F>(f));
} It looks like the emitted export declare function arrayLike<F extends AnySafeMapper>(f: F): (FluentMapper<m.ArrayLikeMapper<F>>); Just one line! I wonder if I will still get that error you pointed out, now that I have explicit return type annotations,
|
Yeah, I noticed in the declaration emit that it seemed to be inlining a lot of stuff for which type aliases exist. I’m honestly not too familiar with how declaration emit works, but it seems like what you found is definitely relevant—I confirmed that tsc can understand the declaration emit of what’s currently in master. When I originally pinged you, I thought maybe there was actually a type error in your library, but it looks like it’s something to do with declaration emit. |
@typescript-bot test this |
Heya @andrewbranch, I've started to run the extended test suite on this PR at 174c0b8. You can monitor the build here. It should now contribute to this PR's status checks. |
Heya @andrewbranch, I've started to run the parallelized Definitely Typed test suite on this PR at 174c0b8. You can monitor the build here. It should now contribute to this PR's status checks. |
I think I’ve whittled it down to nearly the smallest possible repro: And here’s the twist. If you paste In fact, I’ve replaced And the error still occurs, as long as the import type is used! I thought I was onto something, so I changed the import path to be @weswigham any clues now? |
Well! That's interesting! Almost certainly it's because we're doing something too eagerly in import type resolution then. |
Clarification: the circularity error I added repros with this, but the stack size crash doesn’t 🤦♂ |
The difference between the import type and in-file alias is still concerning, IMO. |
Agreed. I think I’m going to prioritize this behind the rest of my 3.6 bugs, but plan to come back to it due to that very strange discrepancy. Just went back to the original declaration files and determined that, yep, if you just import |
Am I still needed for anything? I kind of lost track because it seemed like you got the circularity thing down. Do I still need to try and reproduce the stack size crash? |
Nope, thanks for the help @AnyhowStep :) |
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.
Nice job tracking down the root cause! I like the smallness of the change.
Fixes #31824
In the provided repro, a large circular code path something like this occurs:
Unfortunately, I’m unable to figure out logically what’s happening because the declaration emit is so huge and convoluted that no human can understand it. As such, I have no idea where to begin to try writing a test for this, but the repro no longer crashes and existing tests pass ¯\(ツ)/¯UPDATE, I FIGURED IT OUT
The issue occurs under these exact circumstances:
The reason is that we weren’t properly short circuiting when trying to determine whether
Name
itself is a reference toX
. In trying to figure that out, we ask for the type ofName
, which ends up asking for the type of the wholeimport("./name").Name<X>
, which tries to get the type of ofX
, which tries to resolve the type ofT<X>
, which we were already doing. Q.E.D. Drops mic. 🎤