Skip to content
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

Regression: DTS emit #38356

Closed
Jack-Works opened this issue May 6, 2020 · 5 comments · Fixed by #38368
Closed

Regression: DTS emit #38356

Jack-Works opened this issue May 6, 2020 · 5 comments · Fixed by #38368
Assignees
Labels
Domain: Declaration Emit The issue relates to the emission of d.ts files Domain: JavaScript The issue relates to JavaScript specifically Domain: Type Display Bugs relating to showing types in Quick Info/Tooltips, Signature Help, or Completion Info Needs Investigation This issue needs a team member to investigate its status.

Comments

@Jack-Works
Copy link
Contributor

Works in TS 3.9.0 beta but not in nightly (4.0.0-dev.20200505)

Code:

class X {
    /**
      * Cancels the request, sending a cancellation to the other party
      * @param {Object} error __auto_generated__
      * @param {string?} error.reason the error reason to send the cancellation with
      * @param {string?} error.code the error code to send the cancellation with
      * @returns {Promise.<*>} resolves when the event has been sent.
      */ 
    async cancel({reason = "User declined", code = "m.user"} = {}) {}
}

Playground:

https://www.typescriptlang.org/v2/en/play?ts=4.0.0-dev.20200505&useJavaScript=true#code/MYGwhgzhAEAa0G8CwAoa7oHoBU3UY22gGEwA7YAUxBgBcALS6AJ0oEcBXSiWgGmgiUyAEwCWZAObQw0YOSohwtUQHsy0Wio2NoKhpWbQADmGa0AnvgLQiAARPMwAW0QB5AEYArSsFoBfaANmFUMAfVCwDk1QiSEDMFpKYXCrAjsHZ0QeZnEJAH4AoJCAOlZINW0mIsMyiArNASFhStl5aiVVdQB3UQZUwmh7U0yEbNyCwOZg5mLgFWEmfUnp2XnFrUERFrkKdoTO6B6+tGs7VloOZjIYBAAFYKdRQWKAHmwAPgDWOpAAN25Dox1EtKP8yLRoPRINB3JQhI1wcV+uhsJhoP1IOYKK1diAABQIWoVAC80AARABVQSGBagcRJMn8OYLaCkslOYocalkgKkhB+ACUiD8qFQfiAA

Old output:

declare class X {
    /**
      * Cancels the request, sending a cancellation to the other party
      * @param {Object} error __auto_generated__
      * @param {string?} error.reason the error reason to send the cancellation with
      * @param {string?} error.code the error code to send the cancellation with
      * @returns {Promise.<*>} resolves when the event has been sent.
      */
    cancel({ reason, code }?: {
        reason: string;
        code: string;
    }): Promise<any>;
}

New output (invalid):

declare class X {
    /**
      * Cancels the request, sending a cancellation to the other party
      * @param {Object} error __auto_generated__
      * @param {string?} error.reason the error reason to send the cancellation with
      * @param {string?} error.code the error code to send the cancellation with
      * @returns {Promise.<*>} resolves when the event has been sent.
      */
    cancel({ reason, code }?: 
     * @param {?string} error.reason the error reason to send the cancellation with
     * @param {?string} error.code the error code to send the cancellation with
    ): Promise<any>;
}
@Jack-Works
Copy link
Contributor Author

Another problem:

/**
 * @param {Array} x
 */
function x(x) {}
/**
 * @param {Promise} x
 */
function y(x) {}

In 3.9.0 beta:

/**
 * @param {Array} x
 */
declare function x(x: any[]): void;
/**
 * @param {Promise} x
 */
declare function y(x: Promise<any>): void;

In nightly:

/**
 * @param {Array} x
 */
declare function x(x: Array): void;
/**
 * @param {Promise} x
 */
declare function y(x: Promise): void;

Notice the missing generics make it invalid

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label May 6, 2020
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.0 milestone May 6, 2020
@DanielRosenwasser DanielRosenwasser added Domain: Declaration Emit The issue relates to the emission of d.ts files Domain: JavaScript The issue relates to JavaScript specifically Domain: Type Display Bugs relating to showing types in Quick Info/Tooltips, Signature Help, or Completion Info labels May 6, 2020
@DanielRosenwasser
Copy link
Member

@weswigham this is a 3.9 regression - I think it'd be best to just never reuse type nodes if they come from a JS file.

@weswigham
Copy link
Member

@DanielRosenwasser too late - #38368 has the fix. We have code explicitly mapping jsdoc constructs to ts type constructs, it was just missing the jsdoc object literal form, and didn't account for how we allow missing type parameters (even without defaults) in JS.

@DanielRosenwasser
Copy link
Member

My fear is just that there's another case we haven't covered. The assumption is that TS types are always "well formed" for declaration emit, but JS nodes aren't. As long as we're confident that there aren't other cases that we've missed, #38368 should be sufficient.

@weswigham
Copy link
Member

As we add support for more jsdoc forms, we should check that we actually support declaration emit for those forms, same as we do for TS type nodes. If we could exhaustively check for node kinds so it's typesystem enforced, I would, but, ah, something something, compiler performance compiling itself is still kinda bad when Node is a union of all possible kinds?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Declaration Emit The issue relates to the emission of d.ts files Domain: JavaScript The issue relates to JavaScript specifically Domain: Type Display Bugs relating to showing types in Quick Info/Tooltips, Signature Help, or Completion Info Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
4 participants