Skip to content

Commit

Permalink
Fix JSC_DUP_VAR_DECLARATION_TYPE_MISMATCH in externs
Browse files Browse the repository at this point in the history 
PiperOrigin-RevId: 588711526
  • Loading branch information
@copybara-github
tsjs-language-eng authored and copybara-github committed Dec 18, 2023
1 parent d5c2921 commit 41cf61e
Show file tree
Hide file tree
Showing 30 changed files with 504 additions and 189 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
13 changes: 1 addition & 12 deletions src/externs.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -295,24 +295,13 @@ export function generateExterns(
* interface Foo { x: number; }
* interface Foo { y: number; }
* we only want to emit the "\@record" for Foo on the first one.
*
* The exception are variable declarations, which - in externs - do not assign a value:
* /.. \@type {...} ./
* var someVariable;
* /.. \@type {...} ./
* someNamespace.someVariable;
* If a later declaration wants to add additional properties on someVariable, tsickle must still
* emit an assignment into the object, as it's otherwise absent.
*/
function isFirstValueDeclaration(decl: ts.DeclarationStatement): boolean {
if (!decl.name) return true;
const sym = typeChecker.getSymbolAtLocation(decl.name)!;
if (!sym.declarations || sym.declarations.length < 2) return true;
const earlierDecls = sym.declarations.slice(0, sym.declarations.indexOf(decl));
// Either there are no earlier declarations, or all of them are variables (see above). tsickle
// emits a value for all other declaration kinds (function for functions, classes, interfaces,
// {} object for namespaces).
return earlierDecls.length === 0 || earlierDecls.every(ts.isVariableDeclaration);
return earlierDecls.length === 0 || earlierDecls.every(d => ts.isVariableDeclaration(d) && d.getSourceFile() !== decl.getSourceFile());
}

/** Writes the actual variable statement of a Closure variable declaration. */
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
Loading

0 comments on commit 41cf61e

Please sign in to comment.