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

expando fields not added to symbol exports #31778

Open
JoostK opened this issue Jun 5, 2019 · 2 comments
Open

expando fields not added to symbol exports #31778

JoostK opened this issue Jun 5, 2019 · 2 comments
Assignees
Labels
Needs Investigation This issue needs a team member to investigate its status.

Comments

@JoostK
Copy link
Contributor

JoostK commented Jun 5, 2019

TypeScript Version: 3.4.0, 3.5.0, master (7dc1f40)

Search Terms:
expando, iife, umd, exports

Code

Given the following code, similar to an UMD file:

(function(){
    var A = (function () {
        function A() {}
        return A;
    }());
    A.expando = true;
}());

Expected behavior:

The ts.Symbol of the outer A should have expando in it.

Actual behavior:

The ts.Symbol of the outer A does not have expando in it. When the declaration of A is at the top-level, without the "UMD wrapper", it works properly:

var A = (function () {
    function A() {}
    return A;
}());
A.expando = true;

I have researched what is preventing the expando static prop to be added to exports, and it is due to when a ts.Symbol is considered for expando properties, here:

/**
* Javascript expando values are:
* - Functions
* - classes
* - namespaces
* - variables initialized with function expressions
* - with class expressions
* - with empty object literals
* - with non-empty object literals if assigned to the prototype property
*/
function isExpandoSymbol(symbol: Symbol): boolean {
if (symbol.flags & (SymbolFlags.Function | SymbolFlags.Class | SymbolFlags.NamespaceModule)) {
return true;
}
const node = symbol.valueDeclaration;
if (node && isCallExpression(node)) {
return !!getAssignedExpandoInitializer(node);
}
let init = !node ? undefined :
isVariableDeclaration(node) ? node.initializer :
isBinaryExpression(node) ? node.right :
isPropertyAccessExpression(node) && isBinaryExpression(node.parent) ? node.parent.right :
undefined;
init = init && getRightMostAssignedExpression(init);
if (init) {
const isPrototypeAssignment = isPrototypeAccess(isVariableDeclaration(node) ? node.name : isBinaryExpression(node) ? node.left : node);
return !!getExpandoInitializer(isBinaryExpression(init) && init.operatorToken.kind === SyntaxKind.BarBarToken ? init.right : init, isPrototypeAssignment);
}
return false;
}

Specifically, the symbol's flag of A inside the UMD wrapper are not sufficient to take the early-return in the first statement, whereas with A at the top-level it has been assigned appropriate flags.

Because the appropriate flags are not present, the code structure is analyzed to determine if the symbol should be classified as expando. Specifically, the relevant code is in getExpandoInitializer:

/**
* Recognized expando initializers are:
* 1. (function() {})() -- IIFEs
* 2. function() { } -- Function expressions
* 3. class { } -- Class expressions
* 4. {} -- Empty object literals
* 5. { ... } -- Non-empty object literals, when used to initialize a prototype, like `C.prototype = { m() { } }`
*
* This function returns the provided initializer, or undefined if it is not valid.
*/
export function getExpandoInitializer(initializer: Node, isPrototypeAssignment: boolean): Expression | undefined {
if (isCallExpression(initializer)) {
const e = skipParentheses(initializer.expression);
return e.kind === SyntaxKind.FunctionExpression || e.kind === SyntaxKind.ArrowFunction ? initializer : undefined;
}
if (initializer.kind === SyntaxKind.FunctionExpression ||
initializer.kind === SyntaxKind.ClassExpression ||
initializer.kind === SyntaxKind.ArrowFunction) {
return initializer as Expression;
}
if (isObjectLiteralExpression(initializer) && (initializer.properties.length === 0 || isPrototypeAssignment)) {
return initializer;
}
}

From this function, it becomes clear why the symbol fails to be recognized as expando symbol: the used IIFE syntax deviates from the syntax that is accounted for in getExpandoInitializer. When changing the sample into the following, it does indeed work as expected:

(function(){
    var A = (function () {
        function A() {}
        return A;
    })(); // <-- The difference is here
    A.expando = true;
}());

Unfortunately however, downleveled ES5 code does use the syntax that is not accounted for.

Playground Link:

Can't be replicated in the playground, but copying the code into https://ts-ast-viewer.com, setting the script kind to JS and inspecting the Symbol of the outer A declaration shows that its exports are empty. The alternative IIFE syntax does have the proper exports.

Related Issues:
n/a

Background info
This is an issue for Angular's Compatibility Compiler, which uses the TypeScript compiler to parse JavaScript bundles in various formats and uses the symbol information to reason about the code. PR angular/angular#30795 now contains a hack to patch TS's getExpandoInitializer, which does indeed resolve the issue.

@JoostK
Copy link
Contributor Author

JoostK commented Jun 5, 2019

I tried to create a testcase tests/cases/conformance/salsa/typeFromPropertyAssignment38.ts to replicate the issue, however the following didn't work out:

// @noEmit: true
// @allowJs: true
// @checkJs: true
// @noImplicitAny: true
// @Filename: a.js
(function(){
    var A = (function () {
        function A() {}
        return A;
    }());
    A.expando = true;
    exports.A = A;
}());

// @Filename b.js
var a = require('./a');
a.A.expando;

It has the exact same golden files compared to the alternative IIFE syntax.

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Jun 13, 2019
@RyanCavanaugh
Copy link
Member

@sandersn should we take a PR to drill through IIFEs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
Development

No branches or pull requests

3 participants