Skip to content

Improve control flow analysis of type assertions #26679

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 3 commits into from
Aug 28, 2018

Conversation

ahejlsberg
Copy link
Member

Fixes #26655.

@@ -21698,15 +21698,19 @@ namespace ts {
* to cache the result.
*/
function getTypeOfExpression(node: Expression, cache?: boolean) {
const expr = skipParentheses(node);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can do this quite like this in this codepath - this means this fast path will cause us to skip jsdoc type assertions in JS, which look like /* @type {Type} */(expression).

Copy link
Member

Choose a reason for hiding this comment

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

Same example as repro, but in JS:

// @ts-check
/**
* @typedef {{message: {id: string}}} DataShape
*/

/**
* @param {string | number} id
* @return {any}
*/
function getObject(id) {
  return {} as any
}

;(() => {
  /* @type {string | number} */
  let id = 'a'
  while (1) {
    const data = /* @type {DataShape} */(getObject(id))
    const message = data.message
    id = message.id
  }
})()

Copy link
Member Author

Choose a reason for hiding this comment

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

We can do it this way, it just means that we won't bail out early during control flow analysis. It's simply a matter of how many cases we feel are worthwhile handling in getTypeOfExpression. In a TypeScript file it matters because you'll get an error with --noImplicitAny. In a JavaScript file it just means another any, which are a dime a dozen already, so to speak. So, I think it is fine.

Copy link
Member

Choose a reason for hiding this comment

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

noImplicitAny still works in JS files with checkJs, so the bug does still exist in JS then, it just has a smaller audience.

@ahejlsberg ahejlsberg merged commit ac0d5da into master Aug 28, 2018
@ahejlsberg ahejlsberg deleted the improveGetTypeOfExpression branch August 28, 2018 01:30
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