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

Do not allow freshness to move errors out of the current file, ensure json documents are deeply unfreshened and fully widened #35048

Merged

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Nov 11, 2019

Fixes #34997

So, the two issues at play:

  1. The error was only reported on the second call. This was because the error span was moved into the json document and then discarded (which is why the error had no elaboration - it had already been elaborated in the discarded error). We now have checks in the error span moving code that ensure the span we're moving to is in the same file as the input location, at least, so we'll not discard any errors like that again. (Side note: the fresh literal span moving code is much less safe than the other span moving code, as it looks up locations from symbols, rather than the AST - it would be nicer to remake it to be more like checkTypeRelatedToAndOptionallyElaborate and only traverse the AST)
  2. That there was an error at all was caused by a bug in how object literal freshness freshness was performed for json documents. getRegularTypeOfObjectLiteral is used in checking in a few places to get a relatively shallow unfreshed object, while getWidenedType does full deep unfreshening, accounting for arrays and object contexts (do note: these are cached separately, as they are observably different transforms). We were using getRegularTypeOfObjectLiteral outside of the isRelatedTo context it is appropriate within, and should have used getWidenedType instead, and now we do. This should also have the side effect of producing the nicer context-sensitive object types we produce during assignments from json document types, too (as can be seen in the jsDeclarationsJson.types baseline).

… json documents are deeply unfreshened and fully widened
@fatcerberus
Copy link

Just because I’m curious...

This was because the error span was moved into the json document and then discarded

Since the error was... erroneous, and caused by freshness checks, what prevented the second error from being discarded in the same way as the first?

@weswigham
Copy link
Member Author

The second error used the cached relationship result, which skips the expanded elaboration and error span moving code.

@weswigham weswigham merged commit aa39080 into microsoft:master Nov 12, 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.

Importing arrays from JSON results in incorrect type checking
3 participants