Skip to content

Fix resolution of properties from prototype assignment in JS #29302

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

PranavSenthilnathan
Copy link
Member

Fixes #26885

>d : { (): void; prototype: {}; }
>function() {} : { (): void; prototype: {}; }
>d : typeof d
>function() {} : typeof d

=== tests/cases/conformance/salsa/b.js ===
a.d.prototype = {};
Copy link
Member

Choose a reason for hiding this comment

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

interesting...the change in type display indicates that we're now treating a.d as a class. I'm pretty sure that's because of the prototype assignment, where previously we only checked for members on the symbol.

(we print the type of 'a' as 'typeof a' because the expando assignments turn it into a namespace.)



==== tests/cases/conformance/jsdoc/bug27346.js (2 errors) ====
==== tests/cases/conformance/jsdoc/bug27346.js (1 errors) ====
/**
* @type {MyClass}
~~~~~~~~~~~~~~~
!!! error TS8030: The type of a function declaration must match the function's signature.
Copy link
Member

Choose a reason for hiding this comment

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

I guess the second error goes away because getJSClassType doesn't have const valueType = getTypeOfSymbol() any more? Not entirely sure.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, it might be worthwhile to check whether we have any other error baselines that include the "JSDoc type circularly references itself".

Copy link
Member Author

Choose a reason for hiding this comment

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

No other baselines had this error, and the original repro from the #27346 doesn't give this error anymore. I removed the circularity guard and no baselines changed, so I'll take that as part of this change too.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Looks almost too good to be true! Please do check that we have test coverage of the circular jsdoc type error before merging though.

@DanielRosenwasser
Copy link
Member

Just as a heads up, your commits don't seem to be associated with your GitHub account. While this isn't technically a problem, you might care if you want more appropriate attribution. You can either make sure your GitHub account is associated with the email address you're using for your commits, or rebase and amend your commits to fix the author name and email.

@PranavSenthilnathan PranavSenthilnathan merged commit d38c616 into microsoft:master Jan 19, 2019
errendir added a commit to errendir/TypeScript that referenced this pull request Jan 20, 2019
* origin/master: (64 commits)
  Fix resolution of properties from prototype assignment in JS (microsoft#29302)
  Include all flow nodes made within `try` blocks as antecedents for `catch` or `finally` blocks (microsoft#29466)
  Don't treat interfaces as implementations
  Make the relationship between partial mapped types and the empty object not apply for subtype relationship (microsoft#29384)
  Add missing arity check on second inference pass (microsoft#29386)
  renames
  add missing type annotation
  PR feedback
  Illustrate a case that isn't handled correctly
  Add fourslash tests
  Consider JSX namespace imports when moving statements between files
  Fix gulp builds not building some targets
  Update user baselines (microsoft#29444)
  Add opt-in user preference for prefix and suffix text on renames (microsoft#29314)
  Fake up value declaration for synthetic jsx children symbol so they get excess property checked (microsoft#29359)
  Add regression test. (microsoft#29433)
  Elaborate jsx children elementwise (microsoft#29264)
  Add regression test
  PR feedback
  Fix trailing whitespace
  ...
errendir added a commit to errendir/TypeScript that referenced this pull request Jan 20, 2019
* origin/master: (64 commits)
  Fix resolution of properties from prototype assignment in JS (microsoft#29302)
  Include all flow nodes made within `try` blocks as antecedents for `catch` or `finally` blocks (microsoft#29466)
  Don't treat interfaces as implementations
  Make the relationship between partial mapped types and the empty object not apply for subtype relationship (microsoft#29384)
  Add missing arity check on second inference pass (microsoft#29386)
  renames
  add missing type annotation
  PR feedback
  Illustrate a case that isn't handled correctly
  Add fourslash tests
  Consider JSX namespace imports when moving statements between files
  Fix gulp builds not building some targets
  Update user baselines (microsoft#29444)
  Add opt-in user preference for prefix and suffix text on renames (microsoft#29314)
  Fake up value declaration for synthetic jsx children symbol so they get excess property checked (microsoft#29359)
  Add regression test. (microsoft#29433)
  Elaborate jsx children elementwise (microsoft#29264)
  Add regression test
  PR feedback
  Fix trailing whitespace
  ...
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.

3 participants