Skip to content

Don't check an expression if it was checked cached #22580

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

Closed
wants to merge 3 commits into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Mar 14, 2018

Fixes #22578

Sequel to #22491 -- checking for deferredNodes duplicates catches more than checking for deferredUnusedIdentifierNodes duplicates. Without the change to checkExpression, the new assertion would fire on several tests where nodes were checked multiple times.

@ghost ghost force-pushed the useCheckCache branch from 0119b88 to cf7805d Compare March 14, 2018 20:31
@ghost ghost force-pushed the useCheckCache branch from cf7805d to ffb4dd5 Compare March 14, 2018 20:35
@ghost
Copy link
Author

ghost commented Mar 14, 2018

@sandersn the test failure reduces to:

// @allowJs: true
// @checkJs: true
// @noEmit: true
// @filename: index.js
var c;
c = class { };
new c();

This error only happens in js. It looks like it's doing a non-cached check two times. I've looked through the callstack and can't find why this is js-specific.

@sandersn
Copy link
Member

Binding might be a factor? Something like c = class {} might add a namespace meaning to c. Although I don't think it does unless the binder sees property assignments like c.p = {}.

@ghost
Copy link
Author

ghost commented Mar 14, 2018

Don't know why that was js-specific, but the problem was that getAssignedTypeOfBinaryExpression calls getTypeOfExpression which does a full check. Fixed by adding a CheckMode.JustType based on this comment:

Ideally, the entire family of checkXXX functions should have a parameter that indicates whether full error checking is required such that we can perform the optimizations locally.

@@ -73,6 +73,8 @@ interface XMLHttpRequest {
}
/* tslint:enable:no-var-keyword */

ts.Debug.currentAssertionLevel = ts.AssertionLevel.Normal;
Copy link
Member

Choose a reason for hiding this comment

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

?

@weswigham
Copy link
Member

I assume this fixes the assertion violation related to the related issues on RWC? (There are other rwc issues right now, but I only think one is related)

@typescript-bot
Copy link
Collaborator

Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it.

@ghost ghost deleted the useCheckCache branch July 16, 2018 17: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