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

Stop an infinite loop when using an exported JSDoc function as a type #38332

Closed
wants to merge 2 commits into from

Conversation

orta
Copy link
Contributor

@orta orta commented May 4, 2020

Fixes #36830

In JS we want to support treating values as types in a bunch of positions, the code in #35057 added the ability for this to work in classes but it was over-reaching and also getting called when you have a function. This caused an infinite loop as the type lookup was the same object and not something which could differ in the way an instance vs the 'class' (e.g. the static stuff) could differ.

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

@sandersn this is like, maybe, the 3rd time I've seen this condition change to fix a bug; should this check more semantic in some way, rather than mostly syntactic? Or are there actually just that many unanticipated syntactic forms?

Moreover is this fix complete? Checking just for the Class flag seems incomplete, as an interface could be merged with the function and namespace to produce a similar shape. Isn't any SymbolFlags.Type meaning sufficient? Or SymbolFlags.Namespace if that's the important characteristic? I don't quite see how a class, specifically, should be the conditional, since the same structure can be made to exist thru a merged function, interface, and namespace, and a similar structure (where a symbol has type, value, and namespace meanings) is created by an Enum.

>c.s : () => void
>c : C
>s : () => void
>c.s : any
Copy link
Member

Choose a reason for hiding this comment

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

this seems wrong, which means that this fix may also need to be restricted to types that come through require

@sandersn
Copy link
Member

sandersn commented May 26, 2020

@weswigham
The intent for this code is to de-alias fake JS aliases to types.

  1. The old code was unconditional and non-obvious -- it always assumed the symbol was an alias, dereferenced it, then if dereferencing changed the symbol, returned it. This overapplied in non-obvious places.
  2. The new code is syntactic and explicit, so it underapplies -- although it only needs to handle two cases right now. I expect a tail of bugs from it, but I think the result will be safer. In this case, I didn't understand that only classes need the additional dereference step since they're incorrectly imported via require as value-only.

tl;dr: There are two problems: (1) an unknown number of cases that were previously being handled (2) the existing cases are tricky for me to understand.

  1. I think you are right. Here's a possible example:
// @filename: lib/index.js
/** @type {import('../folder/index').zzz} */
export function xxx() {
  return 12;
}

// @filename: folder/index.d.ts
export function zzz(): string;
export type zzz = (n: number) => number;

The intent is for xxx to have type (n: number) => number, but today it would not.
Edit: Err wait, that's the opposite of the interesting case; it already has a type meaning and wouldn't run the js-specific code.

  1. Overall, we could get rid of this bug farm by making require produce real aliases. Well, at least we could relocate it to a good neighbourhood and clean it up. That needs a couple up-front tasks:

    a. We should figure out how worthwhile high-quality require support is. Probably worth it' since the 2018 survey we've seen that people write ES imports twice as much as commonjs, but I don't have an idea of how much commonjs we support in absolute terms.
    b. I need to relearn the details of binding of aliases, real and fake, because I'm not sure what work is required to make this happen.

@sandersn sandersn added the For Milestone Bug PRs that fix a bug with a specific milestone label May 26, 2020
@sandersn sandersn assigned sandersn and unassigned orta May 26, 2020
@orta
Copy link
Contributor Author

orta commented Aug 18, 2020

Deprecated by #39770

@orta orta closed this Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

JSDOC @type tag crashes with simple export
4 participants