More special declaration types in JS #21974

sandersn · 2018-02-15T19:17:56Z

Object literals as declarations in JS

Starting in 2.7, Typescript supported empty object literals as namespace declarations in Javascript. Before that, it supported functions and classes as namespace declarations:

// before 2.7
var callable = function() {
}
callable.constant = 1

// after 2.7
var ns = {}
ns.constant = 1

This PR adds support for top-level assignments as declarations. A var or const declaration is not required.

app = {} // does NOT need to be “var app = {}”
app.C = class {
}
app.f = function() {
}
app.prop = 1

IIFEs as namespace declarations

This PR assumes that the IIFE returns a function, class or empty object literal, but does not verify it:

var C = (function () {
  function C(n) {
    this.p = n
  }
  return C;
})();
C.staticProperty = 1

Defaulted declarations

"Defaulted declarations" allow initializers that reference the declared name in the left side of a logical or:

my = window.my || {}
my.app = my.app || {}

Prototype assignment

You can assign an object literal directly to the prototype property. Invididual prototype assignments still work too:

var C = function (p) {
  this.p = p
}
C.prototype = {
  m() {
    console.log(this.p)
  }
}
C.prototype.q = function(r) {
  return this.p === r
}

Nested and merged declarations

Nesting works to any level now, and merges correctly across files. Previously neither was the case. The binder now speculatively creates nested symbols for top-level assignments and then the checker performs an extra merge of symbols with the new JSContainer flag.

Fix function containers

Previously functions weren't marked as a JS container when they didn't have any nested properties. This meant that type resolution didn't work:

var Outer = {}
Outer.Inner = function () { }

/** @type {Outer.Inner} */ // ok, Inner is treated as a type just like class { } is
var inner

Fixes #7632

Previously this only worked cross-file because it was a merge. Now it works anywhere, and locally it is actually binding a new property on the object literal symbol.

Plus add tests for them

It is still super hacky

Less polymorphism, up-to-date generalised names and documentation.

This required fixing the predicates and the avoiding of contextual typing loops. This is now done right, in getContextualTypeOfBinaryExpression. The predicates still need work.

Except for adding a newline at the end of file. That's required by Linux, you know!

Turns out merging was incorrect even for non-nested declarations, but tests didn't catch it before.

Also improve assert message in fourslash.

The add-intermediate-container-symbols loop is still quite confusing, but it's not as bad as before.

Makes typingsInstaller compile without adding dependencies

sandersn · 2018-02-15T19:18:47Z

src/compiler/utilities.ts

                    return SpecialPropertyAssignmentKind.ExportsProperty;
                }
-                if (innerPropertyAccess.name.escapedText === "prototype") {
+                if (lhs.expression.name.escapedText === "prototype") {
                    return SpecialPropertyAssignmentKind.PrototypeProperty;
                }
            }


The preceding changes in this function are just a refactor, except for the following 3 lines.

sandersn · 2018-02-23T18:55:54Z

src/compiler/checker.ts

-                else if (name.kind === SyntaxKind.PropertyAccessExpression) {
-                    left = name.expression;
-                }
-                else {


this else clause can never happen because on line 2004 we just checked that name.kind is QualifiedName or PropertyAccessExpression. So I deleted it.

and module.exports.

Previously, this would assert: ```ts exports.undeclared.n = 1; ``` Because undeclared was never declared in any recognised way. Now it no longer asserts, but does not bind. That's because the full pattern starts with the line `exports = require('./x')` and assumes that x.js declares `undeclared`. I am not sure how to bind this. The new test contains this pattern in case I figure it out.

weswigham · 2018-02-28T01:01:38Z

tests/cases/conformance/salsa/typeFromPropertyAssignment16.ts

+Outer.Inner = function () {}
+Outer.Inner.prototype = {
+    x: 1,
+    m() { }


We should also check/enforce that a __proto__ member in the RHS acts as a base type for the class type (this is also present in the devtools codebase).

Ah, Annex B... 😭

__proto__ was sensible ish in a pre-Object.setPrototypeOf world. :P

But there was Object.create :(

I’ll add it to the Big List of JavaScript We Could Do Better. I don’t wa t to put it in this PR though — I have t touched the inheritance code for a while.

sandersn · 2018-03-07T18:03:30Z

src/compiler/binder.ts

+        }
+
+        function forEachIdentifierInEntityName(e: EntityNameExpression, action: (e: Identifier, symbol: Symbol, k: EntityNameExpression) => Symbol): Symbol {
+            if (isIdentifier(e)) {


should handle module.exports and exports as a terminal case instead of in the callers

mhegazy · 2018-03-07T18:10:10Z

src/compiler/binder.ts

-            declareSymbol(file.symbol.exports, file.symbol, <PropertyAccessExpression>node.left, SymbolFlags.Property | SymbolFlags.ExportValue, SymbolFlags.None);
+            const lhs = node.left as PropertyAccessEntityNameExpression;
+            const symbol = forEachIdentifierInEntityName(lhs.expression, (id, original, e) => {
+                if (isExportsOrModuleExportsOrAlias(file, e) || (isIdentifier(e) && e.escapedText === "module" && original === undefined)) {


I do not think we need to handle "module" in a special way. we probably should make forEachIdentifierInEntityName handle the terminal case correctly being identifier | exports | module.exports, and return the correct symbol in each case.

mhegazy · 2018-03-07T19:03:57Z

src/compiler/utilities.ts

+    }
+
+    export function getJavascriptInitializer(initializer: Expression) {
+        if (isCallExpression(initializer)) {


we have a function getImmediatelyInvokedFunctionExpression that is almost the same

It is backwards: You give it a function and it climbs up past parentheses to check with there is a parent that is a CallExpression. This code starts with a CallExpression, climbs down past parentheses, and checks whether the expression is a function. I don't think it's worth undoing the work just to use getImmeidatelyInvokedFunctionExpression. It might be worthwhile to put this in utilities, except I'm not sure what to call it.

mhegazy · 2018-03-07T19:06:09Z

src/compiler/utilities.ts

+        }
+        if (isObjectLiteralExpression(initializer) &&
+            (initializer.properties.length === 0 ||
+             isBinaryExpression(initializer.parent) && isPropertyAccessExpression(initializer.parent.left) && initializer.parent.left.name.escapedText === "prototype")) {


kinda weird to be checking up the parent chain here.. can we do it higher up instead, e.g. getDeclaredJavascriptInitializer

We only check up the parent chain because non-empty object literals are allowed for protoype assignments. I think it is a little cleaner to calculate whether this is true and pass it in, although it's wordier, so I'll go with that.

As we discussed, I'll probably remove most restrictions on initializer kind soon anyway, so these functions will probably be simpler soon.

mhegazy · 2018-03-07T19:06:39Z

src/compiler/utilities.ts

+            (getJavascriptInitializer(node.parent.right) || getDefaultedJavascriptInitializer(node.parent.left as EntityNameExpression, node.parent.right));
+    }
+
+    export function getJavascriptInitializer(initializer: Expression) {


Please add a comment listing the patterns that his function is supporting.

mhegazy · 2018-03-07T19:12:07Z

src/compiler/utilities.ts

+        }
+    }
+
+    export function getAssignedJavascriptInitializer(node: Node) {


where else do we need the BarBartoken handling that is not in getDefaultedJavascriptInitializer? can we consolidate this in one place.

Nowhere, it's just for defaulted initializers. I don't think the property access check is strictly needed, so I'll just move the BarBarToken check into getDefaultedJavascriptInitializer.

mhegazy · 2018-03-07T19:12:46Z

src/compiler/utilities.ts

+        }
+    }
+
+    function getDefaultedJavascriptInitializer(name: EntityNameExpression, initializer: Expression) {


can you add comments on all of these functions.. they have very similar names.

mhegazy

Also we need to handle the crashes you were running into before pushing this one.

1. Add documentation 2. Better organisation of concerns in utility functions 3. Better handling of module.exports and exports in the binder's new code.

sandersn · 2018-03-08T00:27:57Z

I addressed the comments and will take a look at the crashes tomorrow. I have fairly compact repros that should be easy to package as fourslash tests.

Note that the error location is misleading because it's reported inside the merge step for the js initializer.

when the JS symbol is a JS initializer

sandersn added 29 commits November 29, 2017 11:36

JS Object literal assignments are declarations

8bd66a0

Previously this only worked cross-file because it was a merge. Now it works anywhere, and locally it is actually binding a new property on the object literal symbol.

Test:js object literal assignment as declaration

c6a7751

Merge branch 'master' into js-object-literal-assignments-as-declarations

4f07f58

Support var x = x || {} declarations in JS

9e8d59c

Test:basic var x = x || {} support in JS

a51bce0

Support o.x = o.x || {} assignments in JS

2f8c237

Plus add tests for them

Test:o.x = o.x || {} assignments in JS

7e3fdc2

Improve contextual type skip in checkObjectLiteral

e0596ad

It is still super hacky

Update baselines

d3b02be

getSpecialPropertyAssignmentKind uses type guards

4998b99

Refactor JS static property assignment binding

d0b08a2

Less polymorphism, up-to-date generalised names and documentation.

Recursive object-literal-assignment declarations

b0aebb4

4-nested object-literal assignment works in JS

a09c239

Support function/class in JS nested decls

8ac94f5

This required fixing the predicates and the avoiding of contextual typing loops. This is now done right, in getContextualTypeOfBinaryExpression. The predicates still need work.

Return baselines to original state

33c084f

Except for adding a newline at the end of file. That's required by Linux, you know!

Allow window. prefix in default-assignment JS decl

61ea026

Fix bogus jsdoc error

15554d7

Update tests and baselines

03d155f

Correctly merge JS decls

fc08e20

Turns out merging was incorrect even for non-nested declarations, but tests didn't catch it before.

Refactor binder and update baselines.

88c67fa

Also improve assert message in fourslash.

Restrict declaration initializers too

76a9ac4

Clean up bindPropertyAssignment

bad155f

The add-intermediate-container-symbols loop is still quite confusing, but it's not as bad as before.

Clean up js decl code in checker+utilities

0cadfcf

Naming and duplication cleanup

4fdef85

Cleanup in binder: rename and move

d2b933e

Further cleanup

0191b70

Merge branch 'master' into js-object-literal-assignments-as-declarations

8f98c77

Add symbols for just-added test

8bfcf33

Move skipParentheses to utilities

d180117

Makes typingsInstaller compile without adding dependencies

sandersn commented Feb 15, 2018

View reviewed changes

sandersn added 2 commits February 23, 2018 09:24

Merge branch 'master' into js-object-literal-assignments-as-declarations

aa6b76f

Update chrome-devtools baseline

4099d48

sandersn commented Feb 23, 2018

View reviewed changes

sandersn added 3 commits February 27, 2018 10:20

Support js nested namespace decls on exports

c3143d2

and module.exports.

Fix lint

125a317

weswigham reviewed Feb 28, 2018

View reviewed changes

DanielRosenwasser mentioned this pull request Mar 2, 2018

Design Meeting Notes, 3/2/2018 #22304

Closed

mhegazy mentioned this pull request Mar 6, 2018

Is it really truly wrong to have an empty array for files in the config? #12762

Closed

sandersn commented Mar 7, 2018

View reviewed changes

mhegazy reviewed Mar 7, 2018

View reviewed changes

sandersn added 2 commits March 7, 2018 14:51

Merge branch 'master' into js-object-literal-assignments-as-declarations

d86440f

Address PR comments

239f214

1. Add documentation 2. Better organisation of concerns in utility functions 3. Better handling of module.exports and exports in the binder's new code.

sandersn added 3 commits March 8, 2018 09:49

Disallow JS/non-JS merge without crashing

04ceb3d

Note that the error location is misleading because it's reported inside the merge step for the js initializer.

Improve error span:duplicate symbols cross-js/ts

35730f2

when the JS symbol is a JS initializer

Merge branch 'master' into js-object-literal-assignments-as-declarations

f8134d0

sandersn merged commit 99d866d into master Mar 8, 2018

sandersn deleted the js-object-literal-assignments-as-declarations branch March 8, 2018 18:51

RyanCavanaugh mentioned this pull request Mar 8, 2018

"Static method" or properties on functions gives rise to issue #22408

Closed

This was referenced Mar 9, 2018

Go to Definition is not working for simple js object #6839

Closed

Go to Definiton does not work in JavaScript namespace #15996

Closed

Imported namespaces aren't recognized in --checkJs #16489

Closed

microsoft locked and limited conversation to collaborators Jul 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More special declaration types in JS #21974

More special declaration types in JS #21974

sandersn commented Feb 15, 2018 •

edited

Loading

sandersn Feb 15, 2018

sandersn Feb 23, 2018

weswigham Feb 28, 2018

Jessidhia Feb 28, 2018

weswigham Feb 28, 2018

Jessidhia Feb 28, 2018

sandersn Feb 28, 2018

weswigham Feb 28, 2018

sandersn Mar 7, 2018

mhegazy Mar 7, 2018

mhegazy Mar 7, 2018

sandersn Mar 7, 2018

mhegazy Mar 7, 2018

sandersn Mar 8, 2018

mhegazy Mar 7, 2018

mhegazy Mar 7, 2018

sandersn Mar 7, 2018

mhegazy Mar 7, 2018

mhegazy left a comment

sandersn commented Mar 8, 2018

More special declaration types in JS #21974

More special declaration types in JS #21974

Conversation

sandersn commented Feb 15, 2018 • edited Loading

Object literals as declarations in JS

IIFEs as namespace declarations

Defaulted declarations

Prototype assignment

Nested and merged declarations

Fix function containers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy left a comment

Choose a reason for hiding this comment

sandersn commented Mar 8, 2018

sandersn commented Feb 15, 2018 •

edited

Loading