Skip to content

Conversation

@a-tarasyuk
Copy link
Contributor

Fixes #40609

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Sep 28, 2020
@jessetrinity
Copy link
Contributor

Why were duplicate type parameters causing the crash?

@a-tarasyuk
Copy link
Contributor Author

The visitor checks all parameters and adds each parameter type to the result. for the following example

type Foo<T> = {
    fn: (a: T, b: T, c: T) => void;
}

ExtractType tries to create a type type NewType<T, T, T> which causes the error. @jessetrinity does that make sense?

@jessetrinity
Copy link
Contributor

Yes, that makes sense. Thanks for the explanation and the PR!

@jessetrinity jessetrinity merged commit 335ed91 into microsoft:master Sep 29, 2020
@DanielRosenwasser
Copy link
Member

I'm still not clear on why it crashes but this seems to fix another future bug.

@andrewbranch
Copy link
Member

At some point it would be good to add a variety of tests that pump template and pattern literal types through codefixes/refactors. When @jessetrinity and I were debugging this, we definitely saw the formatting scanner get tripped up on that node. I still think there might be a missing condition to reScan something. I’m surprised this fixes that crash based on the behavior we saw, but the formatter works in mysterious ways 🤷

@a-tarasyuk
Copy link
Contributor Author

@andrewbranch I'm ok to revert these changes, in favor of adding changes to formatter.

@jessetrinity
Copy link
Contributor

jessetrinity commented Sep 29, 2020

Strictly speaking it doesn't fix the template type bug, #40738 did.

Previous to that, extract type on

type a = `${a}` 

would cause a crash.

You should actually be able to remove the string templates from the tests in this PR and still get the crash previous to this change.

@andrewbranch
Copy link
Member

I'm ok to revert these changes, in favor of adding changes to formatter.

Oh no, these changes are clearly correct, I was just surprised they fixed the bug.

Strictly speaking it doesn't fix the template type bug, #40738 did.

Ah, that makes more sense 👍

@a-tarasyuk
Copy link
Contributor Author

#40738 did

@jessetrinity Yep, I was waiting for the merging of this PR., because my first PR contained changes related to missing nodes and duplicated type references.

@a-tarasyuk
Copy link
Contributor Author

@andrewbranch @jessetrinity Does that make sense to drop the test tests/cases/fourslash/refactorExtractType69.ts?

@jessetrinity
Copy link
Contributor

I thought it was good to have a test on the code that surfaced the bug, but maybe it's a little confusing since there were two bugs..?

If anything, we might want a more minimal test for the template string issue covering with span:

type a = \*a*\`${extractMe}`\*b*\

@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

For Milestone Bug PRs that fix a bug with a specific milestone

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crash on extract to type alias

5 participants