Skip to content

fix(40609): Crash on extract to type alias #40820

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

Merged
merged 1 commit into from
Sep 29, 2020

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*\

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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