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.

Sign up for GitHub

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

Clean up the remainder of TS 5.0 from tsickle. #1496

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Clean up the remainder of TS 5.0 from tsickle. #1496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
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
14 changes: 9 additions & 5 deletions src/clutz.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,15 @@ export function makeDeclarationTransformerFactory(
// Only need to transform file if we needed one of the above additions.
if (!importStmts && !globalBlock) return file;

return ts.factory.updateSourceFile(file, [
...(importStmts ?? []),
...file.statements,
...(globalBlock ? [globalBlock] : []),
]);
return ts.factory.updateSourceFile(
file,
ts.setTextRange(
ts.factory.createNodeArray([
...(importStmts ?? []),
...file.statements,
...(globalBlock ? [globalBlock] : []),
]),
file.statements));
}
};
};
Expand Down
6 changes: 2 additions & 4 deletions src/decorators.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ export function transformDecoratorsOutputForClosurePropertyRenaming(diagnostics:
return (context: ts.TransformationContext) => {
const result: ts.Transformer<ts.SourceFile> = (sourceFile: ts.SourceFile) => {
let nodeNeedingGoogReflect: undefined|ts.Node = undefined;
const visitor: ts.Visitor = (node) => {
const visitor = (node: ts.Node) => {
const replacementNode = rewriteDecorator(node);
if (replacementNode) {
nodeNeedingGoogReflect = node;
Expand All @@ -107,9 +107,7 @@ export function transformDecoratorsOutputForClosurePropertyRenaming(diagnostics:
return ts.visitEachChild(node, visitor, context);
};
let updatedSourceFile =
// TODO: go/ts50upgrade - Remove after upgrade.
// tslint:disable-next-line:no-unnecessary-type-assertion
ts.visitNode(sourceFile, visitor, ts.isSourceFile)!;
ts.visitNode(sourceFile, visitor, ts.isSourceFile);
if (nodeNeedingGoogReflect !== undefined) {
const statements = [...updatedSourceFile.statements];
const googModuleIndex = statements.findIndex(isGoogModuleStatement);
Expand Down
9 changes: 7 additions & 2 deletions src/enum_transformer.ts
View file
Open in desktop
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
Open in desktop
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
137 changes: 52 additions & 85 deletions src/googmodule.ts
View file
Open in desktop
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 Expand Up @@ -1473,7 +1442,7 @@ export function commonJsToGoogmoduleTransformer(
'requireDynamic', createSingleQuoteStringLiteral(imp));
}

const visitForDynamicImport: ts.Visitor = (node) => {
const visitForDynamicImport = (node: ts.Node) => {
const replacementNode = rewriteDynamicRequire(node);
if (replacementNode) {
return replacementNode;
Expand All @@ -1482,9 +1451,7 @@ export function commonJsToGoogmoduleTransformer(
};

if (host.transformDynamicImport === 'closure') {
// TODO: go/ts50upgrade - Remove after upgrade.
// tslint:disable-next-line:no-unnecessary-type-assertion
sf = ts.visitNode(sf, visitForDynamicImport, ts.isSourceFile)!;
sf = ts.visitNode(sf, visitForDynamicImport, ts.isSourceFile);
}

// Convert each top level statement to goog.module.
Expand Down
20 changes: 4 additions & 16 deletions src/jsdoc_transformer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -827,8 +827,6 @@ export function jsdocTransformer(
const updatedBinding = renameArrayBindings(decl.name, aliases);
if (updatedBinding && aliases.length > 0) {
const declVisited =
// TODO: go/ts50upgrade - Remove after upgrade.
// tslint:disable-next-line:no-unnecessary-type-assertion
ts.visitNode(decl, visitor, ts.isVariableDeclaration)!;
const newDecl = ts.factory.updateVariableDeclaration(
declVisited, updatedBinding, declVisited.exclamationToken,
Expand All @@ -846,10 +844,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 Expand Up @@ -1322,8 +1319,6 @@ export function jsdocTransformer(
e, e.dotDotDotToken,
ts.visitNode(e.propertyName, visitor, ts.isPropertyName),
updatedBindingName,
// TODO: go/ts50upgrade - Remove after upgrade.
// tslint:disable-next-line:no-unnecessary-type-assertion
ts.visitNode(e.initializer, visitor) as ts.Expression));
}
return ts.factory.updateArrayBindingPattern(node, updatedElements);
Expand Down Expand Up @@ -1406,22 +1401,15 @@ export function jsdocTransformer(
if (ts.isBlock(node.statement)) {
updatedStatement = ts.factory.updateBlock(node.statement, [
...aliasDecls,
// TODO: go/ts50upgrade - Remove after upgrade.
// tslint:disable-next-line:no-unnecessary-type-assertion
...ts.visitNode(node.statement, visitor, ts.isBlock)!.statements
]);
} else {
updatedStatement = ts.factory.createBlock([
...aliasDecls,
// TODO: go/ts50upgrade - Remove after upgrade.
// tslint:disable-next-line:no-unnecessary-type-assertion
ts.visitNode(node.statement, visitor) as ts.Statement
...aliasDecls, ts.visitNode(node.statement, visitor) as ts.Statement
]);
}
return ts.factory.updateForOfStatement(
node, node.awaitModifier, updatedInitializer,
// TODO: go/ts50upgrade - Remove after upgrade.
// tslint:disable-next-line:no-unnecessary-type-assertion
ts.visitNode(node.expression, visitor) as ts.Expression,
updatedStatement);
}
Expand Down
Loading
Loading