Skip to content

Walk up parenthesized type nodes when looking for the type alias hosting a node #32924

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

weswigham
Copy link
Member

Fixes #32824

@weswigham
Copy link
Member Author

weswigham commented Aug 15, 2019

@RyanCavanaugh @DanielRosenwasser do we want to consider porting this into the 3.6 release, since alias symbols are pretty important for efficient declaration emit?

@weswigham weswigham force-pushed the skip-parens-when-looking-for-type-alias-host branch from 2db26ef to 4f6473a Compare August 15, 2019 22:12
@microsoft microsoft deleted a comment from typescript-bot Aug 15, 2019
@weswigham
Copy link
Member Author

@typescript-bot pack this for @AnyhowStep

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 15, 2019

Heya @weswigham, I've started to run the tarball bundle task on this PR at 4f6473a. You can monitor the build here. It should now contribute to this PR's status checks.

@@ -18,7 +18,7 @@ type F1 = ([a, b, c]) => void;
>c : any

type T2 = ({ a });
>T2 : { a: any; }
>T2 : T2
Copy link
Member

Choose a reason for hiding this comment

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

I can’t tell whether this makes sense because I can’t immediately deduce the rule for type readouts by reading the pre-existing examples below. F2 gets reported as F2 but T3 prints out the tuple literal structure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tuples are interned and don't have alias symbols, objects have alias symbols, so print as their alias if visible.

@typescript-bot
Copy link
Collaborator

Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/40404/artifacts?artifactName=tgz&fileId=AD9D22CF70561BEAF5758E061B61D3FF891ADF664E37C532F5881393CE7DC83202&fileName=/typescript-3.7.0-insiders.20190815.tgz"
    }
}

and then running npm install.

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Aug 15, 2019

Hmm... This will take me a while to test, I think.
This build gives me a compile error.

But both 3.5.1 and [email protected] do not give me compile errors.

The repro is quite simple. Should I file an issue for this?
I don't think it should be a compile error, anyway.


Here's the repro with expected and actual results,

export type ExtendedMapper<HandledInputT, OutputT, ArgsT extends any[]> = (
    (name : string, mixed : HandledInputT, ...args : ArgsT) => OutputT
);

//type a = (name: string, mixed: any, args_0: any) => any
type a = ExtendedMapper<any, any, [any]>;
//type b = (name: string, mixed: any, ...args: any[]) => any
type b = ExtendedMapper<any, any, any[]>;
//3.5.1, [email protected]
//Expected: "y"
//Actual  : "y"
//https://github.com/microsoft/TypeScript/pull/32924#issuecomment-521819476
//https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/40404/artifacts?artifactName=tgz&fileId=AD9D22CF70561BEAF5758E061B61D3FF891ADF664E37C532F5881393CE7DC83202&fileName=/typescript-3.7.0-insiders.20190815.tgz
//Expected: "y"
//Actual  : "n" <-- Intentional?
type test = a extends b ? "y" : "n"

type a2 = (name: string, mixed: any, args_0: any) => any
type b2 = (name: string, mixed: any, ...args: any[]) => any
//3.5.1, [email protected]
//Expected: "y"
//Actual  : "y"
//https://github.com/microsoft/TypeScript/pull/32924#issuecomment-521819476
//https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/40404/artifacts?artifactName=tgz&fileId=AD9D22CF70561BEAF5758E061B61D3FF891ADF664E37C532F5881393CE7DC83202&fileName=/typescript-3.7.0-insiders.20190815.tgz
//Expected: "y"
//Actual  : "y"
type test2 = a2 extends b2 ? "y" : "n"

Playground


For now, my temporary workaround is just,

Extract<F, (name: string, mixed: any, ...args: any[]) => any>

Because I found that a extends b2 ? "y" : "n" resolves to "y" and b2 extends b ? "y" : "n" resolves to "y".

It seems like assignability isn't transitive (even if we ignore any) =P

AnyhowStep added a commit to AnyhowStep/tsql that referenced this pull request Aug 15, 2019
@AnyhowStep
Copy link
Contributor

AnyhowStep commented Aug 15, 2019

Okay, so, I implemented the workaround making use of the fact that assignability isn't transitive,
AnyhowStep/type-mapping@7d57833

And tested what I originally wanted to test.


No more max call stack size exceeded errors!

If you want to test it yourself,

  1. Clone https://github.com/AnyhowStep/tsql/tree/test-ts-pr-32924
  2. npm install (It should also get TS 3.5.1)
  3. npm run build
  4. Open dist/expr-library/operator/make-binary-operator.d.ts
  5. Notice the output is craaazyyy long
  6. npm run test-compile-time
  7. Notice it crashes with max call stack size exceeded
  8. Install TS build from Walk up parenthesized type nodes when looking for the type alias hosting a node #32924 (comment)
  9. npm run build
  10. Open dist/expr-library/operator/make-binary-operator.d.ts
  11. Notice the output is not crazy long anymore!
  12. npm run test-compile-time
  13. It doesn't crash with max call stack size exceeded anymore!
    The tests will fail because the output has changed but that's not important.

Thank you for fixing this so quick!

@@ -10870,7 +10870,11 @@ namespace ts {
}

function getAliasSymbolForTypeNode(node: TypeNode) {
return isTypeAlias(node.parent) ? getSymbolOfNode(node.parent) : undefined;
let host = node.parent;
while (isParenthesizedTypeNode(host)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

<3

@AnyhowStep
Copy link
Contributor

Any chance of a 3.5 port? =x

@weswigham
Copy link
Member Author

Here's the repro with expected and actual results,

(You can make a demo reproing the difference without relying on the behavior in this PR by seeing the difference between the two types

export type ExtendedMapper<HandledInputT, OutputT, ArgsT extends any[]> = (name : string, mixed : HandledInputT, ...args : ArgsT) => OutputT;
export type ExtendedMapperNoAlias<HandledInputT, OutputT, ArgsT extends any[]> = {x: (name : string, mixed : HandledInputT, ...args : ArgsT) => OutputT}["x"];

so while this does affect how that bug manifests in your codebase, I'd like to see another issue opened for it)

I would guess that would happen because the variance measures don't capture the nuance that relating spread types have (specifically the argument count effects that are independent from the type variance)... I think we'd have to propagate an Unreliable marker there.

@weswigham weswigham merged commit a34ac80 into microsoft:master Aug 20, 2019
@weswigham weswigham deleted the skip-parens-when-looking-for-type-alias-host branch August 20, 2019 23:43
@toriningen toriningen mentioned this pull request Aug 25, 2019
timsuchanek pushed a commit to timsuchanek/TypeScript that referenced this pull request Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Question] Parentheses affects declaration emit. Intentional?
5 participants