Skip to content

Commit

Permalink
Fix missing export emit in devmode for enums
Browse files Browse the repository at this point in the history 
PiperOrigin-RevId: 591225622
  • Loading branch information
@frigus02 @copybara-github
frigus02 authored and copybara-github committed Dec 15, 2023
1 parent d5c2921 commit a7bc5bd
Show file tree
Hide file tree
Showing 27 changed files with 501 additions and 173 deletions.
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
"source-map-support": "^0.5.19",
"tslib": "^2.2.0",
"tslint": "^6.1.3",
"typescript": "5.2.2"
"typescript": "5.3.2"
},
"scripts": {
"build": "tsc",
Expand Down
9 changes: 7 additions & 2 deletions src/enum_transformer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
* type resolve ("@type {Foo}").
*/

import {TsickleHost} from 'tsickle';
import * as ts from 'typescript';

import * as jsdoc from './jsdoc';
Expand Down Expand Up @@ -95,7 +96,7 @@ export function getEnumType(typeChecker: ts.TypeChecker, enumDecl: ts.EnumDeclar
/**
* Transformer factory for the enum transformer. See fileoverview for details.
*/
export function enumTransformer(typeChecker: ts.TypeChecker):
export function enumTransformer(host: TsickleHost, typeChecker: ts.TypeChecker):
(context: ts.TransformationContext) => ts.Transformer<ts.SourceFile> {
return (context: ts.TransformationContext) => {
function visitor<T extends ts.Node>(node: T): T|ts.Node[] {
Expand Down Expand Up @@ -180,7 +181,11 @@ export function enumTransformer(typeChecker: ts.TypeChecker):
/* modifiers */ undefined,
ts.factory.createVariableDeclarationList(
[varDecl],
/* create a const var */ ts.NodeFlags.Const)),
/* When using unoptimized namespaces, create a var
declaration, otherwise create a const var. See b/157460535 */
host.useDeclarationMergingTransformation ?
ts.NodeFlags.Const :
undefined)),
node),
node);

Expand Down
131 changes: 50 additions & 81 deletions src/googmodule.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,9 @@ export interface GoogModuleProcessorHost {
* Takes the import URL of an ES6 import and returns the googmodule module
* name for the imported module, iff the module is an original closure
* JavaScript file.
*
* Warning: If this function is present, GoogModule won't produce diagnostics
* for multiple provides.
*/
jsPathToModuleName?(importPath: string): string|undefined;
jsPathToModuleName?
(importPath: string): {name: string, multipleProvides: boolean}|undefined;
/**
* Takes the import URL of an ES6 import and returns the property name that
* should be stripped from the usage.
Expand Down Expand Up @@ -89,7 +87,8 @@ export function jsPathToNamespace(
host: GoogModuleProcessorHost, context: ts.Node,
diagnostics: ts.Diagnostic[], importPath: string,
getModuleSymbol: () => ts.Symbol | undefined): string|undefined {
const namespace = localJsPathToNamespace(host, importPath);
const namespace =
localJsPathToNamespace(host, context, diagnostics, importPath);
if (namespace) return namespace;

const moduleSymbol = getModuleSymbol();
Expand All @@ -105,15 +104,21 @@ export function jsPathToNamespace(
* Forwards to `jsPathToModuleName` on the host if present.
*/
export function localJsPathToNamespace(
host: GoogModuleProcessorHost, importPath: string): string|undefined {
host: GoogModuleProcessorHost, context: ts.Node|undefined,
diagnostics: ts.Diagnostic[], importPath: string): string|undefined {
if (importPath.match(/^goog:/)) {
// This is a namespace import, of the form "goog:foo.bar".
// Fix it to just "foo.bar".
return importPath.substring('goog:'.length);
}

if (host.jsPathToModuleName) {
return host.jsPathToModuleName(importPath);
const module = host.jsPathToModuleName(importPath);
if (!module) return undefined;
if (module.multipleProvides) {
reportMultipleProvidesError(context, diagnostics, importPath);
}
return module.name;
}

return undefined;
Expand Down Expand Up @@ -394,10 +399,7 @@ function getGoogNamespaceFromClutzComments(
findLocalInDeclarations(moduleSymbol, '__clutz_multiple_provides');
if (hasMultipleProvides) {
// Report an error...
reportDiagnostic(
tsickleDiagnostics, context,
`referenced JavaScript module ${
tsImport} provides multiple namespaces and cannot be imported by path.`);
reportMultipleProvidesError(context, tsickleDiagnostics, tsImport);
// ... but continue producing an emit that effectively references the first
// provided symbol (to continue finding any additional errors).
}
Expand All @@ -411,6 +413,15 @@ function getGoogNamespaceFromClutzComments(
return actualNamespace;
}

function reportMultipleProvidesError(
context: ts.Node|undefined, diagnostics: ts.Diagnostic[],
importPath: string) {
reportDiagnostic(
diagnostics, context,
`referenced JavaScript module ${
importPath} provides multiple namespaces and cannot be imported by path.`);
}

/**
* Converts a TS/ES module './import/path' into a goog.module compatible
* namespace, handling regular imports and `goog:` namespace imports.
Expand Down Expand Up @@ -446,27 +457,6 @@ function rewriteModuleExportsAssignment(expr: ts.ExpressionStatement) {
expr);
}

/**
* Checks whether expr is of the form `exports.abc = identifier` and if so,
* returns the string abc, otherwise returns null.
*/
function isExportsAssignment(expr: ts.Expression): string|null {
// Verify this looks something like `exports.abc = ...`.
if (!ts.isBinaryExpression(expr)) return null;
if (expr.operatorToken.kind !== ts.SyntaxKind.EqualsToken) return null;

// Verify the left side of the expression is an access on `exports`.
if (!ts.isPropertyAccessExpression(expr.left)) return null;
if (!ts.isIdentifier(expr.left.expression)) return null;
if (expr.left.expression.escapedText !== 'exports') return null;

// Check whether right side of assignment is an identifier.
if (!ts.isIdentifier(expr.right)) return null;

// Return the property name as string.
return expr.left.name.escapedText.toString();
}

/**
* Convert a series of comma-separated expressions
* x = foo, y(), z.bar();
Expand Down Expand Up @@ -976,13 +966,19 @@ export function commonJsToGoogmoduleTransformer(
// onSubstituteNode callback.
ts.setEmitFlags(arg.right.expression, ts.EmitFlags.NoSubstitution);

// Namespaces can merge with classes and functions. TypeScript emits
// separate exports assignments for those. Don't emit extra ones here.
// Namespaces can merge with classes and functions and TypeScript emits
// separate exports assignments for those already. No need to add an
// extra one.
// The same is true for enums, but only if they have been transformed
// to closure enums.
const notAlreadyExported = matchingExports.filter(
decl => !ts.isClassDeclaration(
decl.declarationSymbol.valueDeclaration) &&
!ts.isFunctionDeclaration(
decl.declarationSymbol.valueDeclaration));
decl.declarationSymbol.valueDeclaration) &&
!(host.transformTypesToClosure &&
ts.isEnumDeclaration(
decl.declarationSymbol.valueDeclaration)));

const exportNames = notAlreadyExported.map(decl => decl.exportName);
return {
Expand Down Expand Up @@ -1147,7 +1143,6 @@ export function commonJsToGoogmoduleTransformer(
return exportStmt;
}

const exportsSeen = new Set<string>();
const seenNamespaceOrEnumExports = new Set<string>();

/**
Expand Down Expand Up @@ -1245,53 +1240,27 @@ export function commonJsToGoogmoduleTransformer(
return;
}

// Avoid EXPORT_REPEATED_ERROR from JSCompiler. Occurs for:
// class Foo {}
// namespace Foo { ... }
// export {Foo};
// TypeScript emits 2 separate exports assignments. One after the
// class and one after the namespace.
// TODO(b/277272562): TypeScript 5.1 changes how exports assignments
// are emitted, making this no longer an issue. On the other hand
// this is unsafe. We really need to keep the _last_ (not the first)
// export assignment in the general case. Remove this check after
// the 5.1 upgrade.
const exportName = isExportsAssignment(exprStmt.expression);
if (exportName) {
if (exportsSeen.has(exportName)) {
stmts.push(createNotEmittedStatementWithComments(sf, exprStmt));
return;
}
exportsSeen.add(exportName);
}

// TODO(b/277272562): This code works in 5.1. But breaks in 5.0,
// which emits separate exports assignments for namespaces and enums
// and this code would emit duplicate exports assignments. Run this
// unconditionally after 5.1 has been released.
if ((ts.versionMajorMinor as string) !== '5.0') {
// Check for inline exports assignments as they are emitted for
// exported namespaces and enums, e.g.:
// (function (Foo) {
// })(Foo || (exports.Foo = exports.Bar = Foo = {}));
// and moves the exports assignments to a separate statement.
const exportInIifeArguments =
maybeRewriteExportsAssignmentInIifeArguments(exprStmt);
if (exportInIifeArguments) {
stmts.push(exportInIifeArguments.statement);
for (const newExport of exportInIifeArguments.exports) {
const exportName = newExport.expression.left.name.text;
// Namespaces produce multiple exports assignments when
// they're re-opened in the same file. Only emit the first one
// here. This is fine because the namespace object itself
// cannot be re-assigned later.
if (!seenNamespaceOrEnumExports.has(exportName)) {
stmts.push(newExport);
seenNamespaceOrEnumExports.add(exportName);
}
// Check for inline exports assignments as they are emitted for
// exported namespaces and enums, e.g.:
// (function (Foo) {
// })(Foo || (exports.Foo = exports.Bar = Foo = {}));
// and moves the exports assignments to a separate statement.
const exportInIifeArguments =
maybeRewriteExportsAssignmentInIifeArguments(exprStmt);
if (exportInIifeArguments) {
stmts.push(exportInIifeArguments.statement);
for (const newExport of exportInIifeArguments.exports) {
const exportName = newExport.expression.left.name.text;
// Namespaces produce multiple exports assignments when
// they're re-opened in the same file. Only emit the first one
// here. This is fine because the namespace object itself
// cannot be re-assigned later.
if (!seenNamespaceOrEnumExports.has(exportName)) {
stmts.push(newExport);
seenNamespaceOrEnumExports.add(exportName);
}
return;
}
return;
}

// Delay `exports.X = X` assignments for decorated classes.
Expand Down
7 changes: 3 additions & 4 deletions src/jsdoc_transformer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -846,10 +846,9 @@ export function jsdocTransformer(
continue;
}
}
const newDecl =
// TODO: go/ts50upgrade - Remove after upgrade.
// tslint:disable-next-line:no-unnecessary-type-assertion
ts.visitNode(decl, visitor, ts.isVariableDeclaration)!;
const newDecl = ts.setEmitFlags(
ts.visitNode(decl, visitor, ts.isVariableDeclaration)!,
ts.EmitFlags.NoComments);
const newStmt = ts.factory.createVariableStatement(
varStmt.modifiers,
ts.factory.createVariableDeclarationList([newDecl], flags));
Expand Down
49 changes: 44 additions & 5 deletions src/ns_transformer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ export function namespaceTransformer(
// transformation fails.
function transformNamespace(
ns: ts.ModuleDeclaration,
mergedDecl: ts.ClassDeclaration|
ts.InterfaceDeclaration): ts.Statement[] {
mergedDecl: ts.ClassDeclaration|ts.InterfaceDeclaration|
ts.EnumDeclaration): ts.Statement[] {
if (!ns.body || !ts.isModuleBlock(ns.body)) {
if (ts.isModuleDeclaration(ns)) {
error(
Expand All @@ -83,10 +83,15 @@ export function namespaceTransformer(
return [ns];
}
const nsName = getIdentifierText(ns.name as ts.Identifier);
const mergingWithEnum = ts.isEnumDeclaration(mergedDecl);
const transformedNsStmts: ts.Statement[] = [];
for (const stmt of ns.body.statements) {
if (ts.isEmptyStatement(stmt)) continue;
if (ts.isClassDeclaration(stmt)) {
if (mergingWithEnum) {
errorNotAllowed(stmt, 'class');
continue;
}
transformInnerDeclaration(
stmt, (classDecl, notExported, hoistedIdent) => {
return ts.factory.updateClassDeclaration(
Expand All @@ -95,12 +100,20 @@ export function namespaceTransformer(
classDecl.members);
});
} else if (ts.isEnumDeclaration(stmt)) {
if (mergingWithEnum) {
errorNotAllowed(stmt, 'enum');
continue;
}
transformInnerDeclaration(
stmt, (enumDecl, notExported, hoistedIdent) => {
return ts.factory.updateEnumDeclaration(
enumDecl, notExported, hoistedIdent, enumDecl.members);
});
} else if (ts.isInterfaceDeclaration(stmt)) {
if (mergingWithEnum) {
errorNotAllowed(stmt, 'interface');
continue;
}
transformInnerDeclaration(
stmt, (interfDecl, notExported, hoistedIdent) => {
return ts.factory.updateInterfaceDeclaration(
Expand All @@ -109,20 +122,39 @@ export function namespaceTransformer(
interfDecl.members);
});
} else if (ts.isTypeAliasDeclaration(stmt)) {
if (mergingWithEnum) {
errorNotAllowed(stmt, 'type alias');
continue;
}
transformTypeAliasDeclaration(stmt);
} else if (ts.isVariableStatement(stmt)) {
if ((ts.getCombinedNodeFlags(stmt.declarationList) &
ts.NodeFlags.Const) === 0) {
error(
stmt,
'non-const values are not supported. (go/ts-merged-namespaces)');
continue;
}
if (!ts.isInterfaceDeclaration(mergedDecl)) {
error(
stmt,
'const declaration only allowed when merging with an interface (go/ts-merged-namespaces)');
continue;
}
transformConstDeclaration(stmt);
} else if (ts.isFunctionDeclaration(stmt)) {
if (!ts.isEnumDeclaration(mergedDecl)) {
error(
stmt,
'function declaration only allowed when merging with an enum (go/ts-merged-namespaces)');
}
transformInnerDeclaration(
stmt, (funcDecl, notExported, hoistedIdent) => {
return ts.factory.updateFunctionDeclaration(
funcDecl, notExported, funcDecl.asteriskToken,
hoistedIdent, funcDecl.typeParameters,
funcDecl.parameters, funcDecl.type, funcDecl.body);
});
} else {
error(
stmt,
Expand All @@ -145,6 +177,12 @@ export function namespaceTransformer(

// Local functions follow.

function errorNotAllowed(stmt: ts.Statement, declKind: string) {
error(
stmt,
`${declKind} cannot be merged with enum declaration. (go/ts-merged-namespaces)`);
}

type DeclarationStatement = ts.Declaration&ts.DeclarationStatement;

function transformConstDeclaration(varDecl: ts.VariableStatement) {
Expand Down Expand Up @@ -365,12 +403,13 @@ export function namespaceTransformer(
}

if (!ts.isInterfaceDeclaration(mergedDecl) &&
!ts.isClassDeclaration(mergedDecl)) {
// The previous declaration is not a class or interface.
!ts.isClassDeclaration(mergedDecl) &&
!ts.isEnumDeclaration(mergedDecl)) {
// The previous declaration is not a class, enum, or interface.
transformedStmts.push(ns); // Nothing to do here.
error(
ns.name,
'merged declaration must be local class or interface. (go/ts-merged-namespaces)');
'merged declaration must be local class, enum, or interface. (go/ts-merged-namespaces)');
return;
}

Expand Down
1 change: 1 addition & 0 deletions src/summary.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ export class FileSummary {
modName: string|undefined;
autochunk = false;
enhanceable = false;
legacyNamespace = false;
moduleType = ModuleType.UNKNOWN;

private stringify(symbol: Symbol): string {
Expand Down
Loading

0 comments on commit a7bc5bd

Please sign in to comment.