Better JS container binding by sandersn · Pull Request #24367 · microsoft/TypeScript

sandersn · 2018-05-23T23:25:59Z

This PR improves the binding of JS containers. It no longer binds function expressions, class expressions, object literals or IIFEs as declarations. Instead, the actual declaration is the one that merges with subsequent special assignment.

For example,

var C = function() {
  this.x = 1
}
C.prototype.y = 3
C.z = 2

Previously put x and y on the members of the function expression and z on its exports. Now, it puts x on the members of the function expression (since it is declared there), y on the members of C and z on the exports of C. When the checker requests the type of the function expression, getTypeOfFuncClassEnumModule checks whether the declaration (C in this case) has any special exports or members and merges them if so. The resulting type is the type of the function expression as well as C.

This has three advantages:

Better type printing in many cases.
Simpler code in bindPropertyAssignment.
Simpler code in mergeSymbol.

Both these pieces of code were previously bug farms.

Also, since the checker now produces the combined types instead of the binder, IIFE type checking is better; the returned type of the IIFE is intersected with an anonymous type containing exports produced by special JS assignments.

Fixes #24110
Fixes #24111

Also update SymbolLinks in getTypeOfFuncClassEnumModule so that the type gets cached correctly.

Also include a couple of improved baselines

The utility is horrible and needs to change, but at least it's in one place. Next step is to make the utility like getDeclarationOfAlias, except getDeclarationOfJSAlias.

Merging even seems to work ok.

Perhaps in the wrong way. I have an idea how to simplify them.

1. It's not completely removed; the checker code in getJavascriptClassType needs to be fixed, among other places. 2. I didn't actually remove the code so that it will be easier to see what used to be there on Monday. Regardless, the code will be much simpler and seems to be mostly improved with very little work so far.

1. Remove a few TODOs 2. Remove extraneous SymbolFlag 3. Simplify isSameDefaultedName

sandersn · 2018-05-24T15:50:53Z

Running Ryan's fuzzer in its original editor-mode configuration doesn't show any oomemory or crashes for over 1,000,000 iterations. I'll run it in get-diagnostics-mode configuration once I change the checker to handle more kinds of Javascript merges in my next PR. (Right now our pre-existing lack of handling crazy merges causes us to hit asserts in various branches of getTypeOfSymbol.)

weswigham · 2018-05-25T00:39:24Z

+                const jsDeclaration = getDeclarationOfJSInitializer(symbol.valueDeclaration);
+                if (jsDeclaration) {
+                    const jsSymbol = getMergedSymbol(jsDeclaration.symbol);
+                    if (jsSymbol && (jsSymbol.exports && jsSymbol.exports.size || jsSymbol.members && jsSymbol.members.size)) {


If jsSymbol === symbol (somehow) does this work need to be done?

No, and it would be really bad because you would be merging symbols with themselves. But it would hit the new assert in mergeSymbol, so it doesn’t happen in our tests at least.

But you make a valid point; getDeclarationOfJSInitializer should (1) check that its node is in a js file (2) check that the provided value declaration is the initializer of the js declaration that it returns.

Fixed. (As is the call to getSymbolOfNode.)

sandersn · 2018-05-25T15:11:38Z

            if (!links.type) {
+                const jsDeclaration = getDeclarationOfJSInitializer(symbol.valueDeclaration);
+                if (jsDeclaration) {
+                    const jsSymbol = getMergedSymbol(jsDeclaration.symbol);


This should just be a call to getsymbolofnode I think.

Change getDeclarationOfJSInitializer to require that the provided js initializer be in a javascript file, and that it is the initializer of the retrieved declaration.

These are cases in a really embarrassing check, in which we admit that the symbol flags steered us wrong and switch to getTypeOfFuncClassEnumModule instead (which never asserts).

sandersn · 2018-05-25T22:42:55Z

I ran Ryan's fuzzer with getSemanticDiagnostics after every iteration after fixing the asserts it found. It didn't show any crashes, oomemory or asserts after 2,500 iterations (it runs a lot slower since in this mode, it checks each generated program.) The fix is small, so I threw it into this PR as well.

Edit: Over 6,500 iterations.

sandersn · 2018-05-30T19:55:41Z

ping @weswigham @mhegazy

weswigham · 2018-05-30T23:28:49Z

+                if (!(target.flags & SymbolFlags.Transient)) {
+                    target = cloneSymbol(target);
+                }
+                Debug.assert(source !== target);


Should this be above the Transitent check, or is it OK for a nontransient target to merge into "itself" (its transient copy)?

Strictly speaking, it doesn't matter, because source will never equal target just after running cloneSymbol. I think it is clearer beforehand though.

weswigham · 2018-05-30T23:30:04Z

+                const jsDeclaration = getDeclarationOfJSInitializer(symbol.valueDeclaration);
+                if (jsDeclaration) {
+                    const jsSymbol = getSymbolOfNode(jsDeclaration);
+                    if (jsSymbol && (jsSymbol.exports && jsSymbol.exports.size || jsSymbol.members && jsSymbol.members.size)) {


Style: Maybe we should add a size helper function that handles undefined like how we have length for arrays?

Let me see how many times we check for null before accessing size. The level of inconvenience seems worth about 5-6 checks before making a helper.

Later: it's over 10 now. I'll add the helper.

weswigham · 2018-05-30T23:31:39Z

+            if (isInJavaScriptFile(node)) {
+                const decl = getDeclarationOfJSInitializer(node);
+                if (decl) {
+                    const jsSymbol = getMergedSymbol(decl.symbol);


Use getSymbolOfNode(decl) - unless you're intentionally avoiding the late bound node check for some reason.

No, I just forgot again. Thanks.

weswigham

Some comments above, but looks good.

sandersn added 21 commits May 17, 2018 10:02

Static assignments to class expressions work

51341c8

Bind static properties of functions too

9dddd3a

Also update SymbolLinks in getTypeOfFuncClassEnumModule so that the type gets cached correctly.

Remove initializer handling:obj literals+type lookup

aab1427

Also include a couple of improved baselines

Fix 1-nested js containers:binding+cross-file merge

009080a

Merge branch 'master' into js/better-initializer-binding

1630863

Consolidate check into one utility

b3bae86

The utility is horrible and needs to change, but at least it's in one place. Next step is to make the utility like getDeclarationOfAlias, except getDeclarationOfJSAlias.

Defaulted assignments now (mostly) work

379e038

Default assignment definitely work, and IIFEs kind of do

a142a1e

n-nested undeclared containers now seem to work

497b1b0

Merging even seems to work ok.

Handle prototype+prototype property assignments

7a1ae1a

Perhaps in the wrong way. I have an idea how to simplify them.

Allow more merges+accept baselines

03461d3

Update more baselines

65d4cfc

Fix js initializer check in bindPropertyAssignment

8c3bd9e

Fix codefixes

95083d6

Merge branch 'master' into js/better-initializer-binding

038695c

Rest of strictNullChecks cleanup + other cleanup

7e87721

1. Remove a few TODOs 2. Remove extraneous SymbolFlag 3. Simplify isSameDefaultedName

Binder cleanup

1557f1b

Checker cleanup

e78d6e9

Almost done with utilities cleanup

81d4979

Utilities cleanup

89412e2

sandersn requested a review from mhegazy May 23, 2018 23:26

weswigham reviewed May 25, 2018

View reviewed changes

sandersn commented May 25, 2018

View reviewed changes

sandersn added 4 commits May 25, 2018 13:00

Require js initializer to be (1) JS (2) initializer

0b13acd

Change getDeclarationOfJSInitializer to require that the provided js initializer be in a javascript file, and that it is the initializer of the retrieved declaration.

Use getSymbolOfNode instead of accessing symbol directly

be7e61d

Ugh. Start over with just test cases

5fbb75a

Handle additional cases in getTypeOfVariableOrParameterOrProperty

5790a06

These are cases in a really embarrassing check, in which we admit that the symbol flags steered us wrong and switch to getTypeOfFuncClassEnumModule instead (which never asserts).

sandersn added 2 commits May 25, 2018 15:56

Add test case for #24111

6fe3f93

Merge branch 'master' into js/better-initializer-binding

2bfbefe

This was referenced May 29, 2018

Assert failure in forEachIdentifierInEntityName with prototype assignment + prototype property assignment #24111

Closed

Assert in getTypeOfVariableOrParameterOrProperty when merging variables and functions #24110

Closed

weswigham reviewed May 30, 2018

View reviewed changes

weswigham approved these changes May 30, 2018

View reviewed changes

Address PR comments

bc0fd99

sandersn merged commit d187de2 into master May 31, 2018

sandersn deleted the js/better-initializer-binding branch May 31, 2018 18:41

microsoft locked and limited conversation to collaborators Jul 31, 2018

Conversation

sandersn commented May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandersn commented May 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandersn commented May 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weswigham left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sandersn commented May 23, 2018 •

edited

Loading

sandersn commented May 25, 2018 •

edited

Loading