From 9027c3bd6027729c25050724096983f6391981b2 Mon Sep 17 00:00:00 2001 From: Derek Xu Date: Mon, 11 Nov 2019 16:04:21 -0800 Subject: [PATCH] Improved handling of invalid identifiers --- lib/base.ts | 17 ++++----- lib/declaration.ts | 24 ++++++------ lib/facade_converter.ts | 71 +++++++++++++++++++++++++++-------- lib/merge.ts | 2 +- lib/module.ts | 2 +- lib/type.ts | 4 +- test/declaration_test.ts | 8 ++-- test/facade_converter_test.ts | 59 +++++++++++++++++++++++++++++ 8 files changed, 140 insertions(+), 47 deletions(-) diff --git a/lib/base.ts b/lib/base.ts index 5d1c5d5..2d86bda 100644 --- a/lib/base.ts +++ b/lib/base.ts @@ -87,21 +87,18 @@ export interface ExtendedInterfaceDeclaration extends ts.InterfaceDeclaration { } export function ident(n: ts.Node): string { - if (!n) return null; - if (ts.isIdentifier(n)) return n.text; - if (n.kind === ts.SyntaxKind.FirstLiteralToken) return (n as ts.LiteralLikeNode).text; + if (ts.isIdentifier(n) || ts.isStringLiteralLike(n)) { + return n.text; + } if (ts.isQualifiedName(n)) { - let leftName = ident(n.left); - if (leftName) return leftName + '.' + ident(n.right); + const leftName = ident(n.left); + if (leftName) { + return leftName + '.' + ident(n.right); + } } return null; } -export function isValidDartIdentifier(text: string) { - const validIdentifierRegExp = new RegExp('^[^0-9_][a-zA-Z0-9_$]*$'); - return validIdentifierRegExp.test(text); -} - export function isFunctionTypedefLikeInterface(ifDecl: ts.InterfaceDeclaration): boolean { return ifDecl.members && ifDecl.members.length === 1 && ts.isCallSignatureDeclaration(ifDecl.members[0]); diff --git a/lib/declaration.ts b/lib/declaration.ts index e99c4fb..dbe7986 100644 --- a/lib/declaration.ts +++ b/lib/declaration.ts @@ -1,7 +1,7 @@ import * as ts from 'typescript'; import * as base from './base'; -import {FacadeConverter} from './facade_converter'; +import {FacadeConverter, identifierCanBeRenamed, isValidIdentifier} from './facade_converter'; import {Transpiler} from './main'; import {MergedMember, MergedParameter, MergedType, MergedTypeParameters} from './merge'; @@ -240,8 +240,9 @@ export default class DeclarationTranspiler extends base.TranspilerBase { this.visitMergingOverloads(decl.members); if (isPropertyBag) { - const propertiesWithValidNames = properties.filter( - p => !ts.isStringLiteral(p.name) || base.isValidDartIdentifier(p.name.text)); + const propertiesWithValidNames = properties.filter((p) => { + return isValidIdentifier(p.name); + }); this.emit('external factory'); this.fc.visitTypeName(name); if (propertiesWithValidNames.length) { @@ -584,10 +585,6 @@ export default class DeclarationTranspiler extends base.TranspilerBase { }); } } break; - case ts.SyntaxKind.StringLiteral: { - let sLit = node; - this.emit(sLit.text); - } break; case ts.SyntaxKind.CallSignature: { let fn = node; this.emit('external'); @@ -834,12 +831,13 @@ export default class DeclarationTranspiler extends base.TranspilerBase { */ private visitProperty( decl: ts.PropertyDeclaration|ts.ParameterDeclaration, isParameter?: boolean) { - const hasValidName = - !ts.isStringLiteral(decl.name) || base.isValidDartIdentifier(decl.name.text); + // Check if the name contains special characters other than $ and _ + const canBeRenamed = identifierCanBeRenamed(decl.name); - // TODO(derekx): Properties with names that contain special characters are currently ignored by - // commenting them out. Determine a way to rename these properties in the future. - this.maybeWrapInCodeComment({shouldWrap: !hasValidName, newLine: true}, () => { + // TODO(derekx): Properties with names that contain special characters other than _ and $ are + // currently ignored by commenting them out. We should determine a way to rename these + // properties using extension members in the future. + this.maybeWrapInCodeComment({shouldWrap: !canBeRenamed, newLine: true}, () => { this.emitProperty({ mode: emitPropertyMode.getter, declaration: decl, @@ -849,7 +847,7 @@ export default class DeclarationTranspiler extends base.TranspilerBase { }); if (!base.isReadonly(decl)) { - this.maybeWrapInCodeComment({shouldWrap: !hasValidName, newLine: true}, () => { + this.maybeWrapInCodeComment({shouldWrap: !canBeRenamed, newLine: true}, () => { this.emitProperty({ mode: emitPropertyMode.setter, declaration: decl, diff --git a/lib/facade_converter.ts b/lib/facade_converter.ts index 8196b97..e9e89c2 100644 --- a/lib/facade_converter.ts +++ b/lib/facade_converter.ts @@ -15,16 +15,53 @@ const FACADE_NODE_MODULES_PREFIX = /^(\.\.\/)*node_modules\//; */ export const DART_RESERVED_NAME_PREFIX = 'JS$'; +/** + * Returns whether or not the given identifier is valid for the type of object it refers to. Certain + * reserved keywords cannot ever be used as identifiers. Other keywords, the built-in identifiers, + * cannot be used as class or type names, but can be used elsewhere. + */ +export function isValidIdentifier(identifier: ts.DeclarationName|ts.PropertyName): boolean { + // Check if this identifier is being used as a class or type name + const parent = identifier.parent; + + const isClassOrTypeName = parent && + (ts.isClassDeclaration(parent) || ts.isInterfaceDeclaration(parent) || ts.isTypeNode(parent)); + + const text = base.ident(identifier); + if (FacadeConverter.DART_RESERVED_WORDS.has(text)) { + return false; + } + if (isClassOrTypeName && FacadeConverter.DART_BUILT_IN_IDENTIFIERS.has(text)) { + return false; + } + + const validIdentifierRegExp = new RegExp('^[^0-9_][a-zA-Z0-9_$]*$'); + return validIdentifierRegExp.test(text); +} + +export function identifierCanBeRenamed(identifier: ts.DeclarationName|ts.PropertyName): boolean { + const text = base.ident(identifier); + const renamableRegExp = new RegExp('^[a-zA-Z0-9_$]*$'); + return renamableRegExp.test(text); +} + /** * Fix TypeScript identifier names that are not valid Dart names by adding JS$ to the start of the * identifier name. */ -export function fixupIdentifierName(text: string): string { - return (FacadeConverter.DART_RESERVED_WORDS.indexOf(text) !== -1 || - FacadeConverter.DART_OTHER_KEYWORDS.indexOf(text) !== -1 || - !base.isValidDartIdentifier(text)) ? - DART_RESERVED_NAME_PREFIX + text : - text; +export function fixupIdentifierName(node: ts.DeclarationName|ts.PropertyName): string { + const text = base.ident(node); + + if (isValidIdentifier(node)) { + // If the name is already valid, it does not need to be changed + return text; + } else if (identifierCanBeRenamed(node)) { + // If the identifier will become valid by prepending JS$ to it, return that name + return DART_RESERVED_NAME_PREFIX + text; + } + // If the name cannot be renamed to be valid, it remains unmodified. The Declaration transpiler + // should have detected the invalid name and wrapped it in a code comment. + return text; } function hasVarArgs(parameters: ts.ParameterDeclaration[]): boolean { @@ -114,7 +151,9 @@ export class NameRewriter { // name. This is an arbitrary but hopefully unsurprising scheme to // generate unique names. There may be classes or members with conflicting // names due to a single d.ts file containing multiple modules. - let candidateName = fixupIdentifierName(parts.slice(i).join('_')); + const candidateIdentifier = ts.createIdentifier(parts.slice(i).join('_')); + candidateIdentifier.parent = node; + const candidateName = fixupIdentifierName(candidateIdentifier); if (library.addName(candidateName)) { // Able to add name to library. let ret = new DartNameRecord(node, candidateName, library); @@ -210,18 +249,18 @@ function removeResolvedTypeArguments(options: TypeDisplayOptions): TypeDisplayOp export class FacadeConverter extends base.TranspilerBase { tc: ts.TypeChecker; // For the Dart keyword list see - // https://www.dartlang.org/docs/dart-up-and-running/ch02.html#keywords - static DART_RESERVED_WORDS = + // https://dart.dev/guides/language/language-tour#keywords + static DART_RESERVED_WORDS: Set = new Set( ('assert break case catch class const continue default do else enum extends false final ' + - 'finally for if in is new null rethrow return super switch this throw true try let void ' + + 'finally for if in is new null rethrow return super switch this throw true try let var void ' + 'while with') - .split(/ /); + .split(/ /)); - // These are the built-in and limited keywords. - static DART_OTHER_KEYWORDS = - ('abstract as async await covariant deferred dynamic export external factory Function get implements import ' + - 'library mixin operator part set static sync typedef yield call') - .split(/ /); + // These are the built-in identifiers. + static DART_BUILT_IN_IDENTIFIERS: Set = new Set( + ('abstract as covariant deferred dynamic export external factory Function get implements import interface' + + 'library mixin operator part set static typedef') + .split(/ /)); private candidateTypes: Set = new Set(); private typingsRootRegex: RegExp; diff --git a/lib/merge.ts b/lib/merge.ts index 7b01283..4339efc 100644 --- a/lib/merge.ts +++ b/lib/merge.ts @@ -429,7 +429,7 @@ export function normalizeSourceFile(f: ts.SourceFile, fc: FacadeConverter, expli undefined { const members = classLike.members as ts.NodeArray; return members.find((member: ts.ClassElement) => { - if (base.ident(member.name) === propName) { + if (member.name && base.ident(member.name) === propName) { return true; } }); diff --git a/lib/module.ts b/lib/module.ts index 48855c8..d68d7e5 100644 --- a/lib/module.ts +++ b/lib/module.ts @@ -145,7 +145,7 @@ export default class ModuleTranspiler extends base.TranspilerBase { return parts.filter((p) => p.length > 0 && p !== '..') .map((p) => p.replace(/[^\w.]/g, '_')) .map((p) => p.replace(/\.dart$/, '')) - .map((p) => FacadeConverter.DART_RESERVED_WORDS.indexOf(p) !== -1 ? '_' + p : p) + .map((p) => FacadeConverter.DART_RESERVED_WORDS.has(p) ? '_' + p : p) .filter((p) => p.length > 0) .join('.'); } diff --git a/lib/type.ts b/lib/type.ts index 33b2a0e..53b6702 100644 --- a/lib/type.ts +++ b/lib/type.ts @@ -47,8 +47,8 @@ export default class TypeTranspiler extends base.TranspilerBase { this.visit(first.left); this.emit('.'); this.visit(first.right); - } else if (ts.isIdentifier(node) || node.kind === ts.SyntaxKind.FirstLiteralToken) { - let text = fixupIdentifierName(base.ident(node)); + } else if (ts.isIdentifier(node) || ts.isStringLiteralLike(node)) { + const text = fixupIdentifierName(node); this.emit(text); } else { return false; diff --git a/test/declaration_test.ts b/test/declaration_test.ts index 7e102af..69406e9 100644 --- a/test/declaration_test.ts +++ b/test/declaration_test.ts @@ -732,15 +732,15 @@ abstract class X { expectTranslate(`interface X { '5abcde': string; }`).to.equal(`@anonymous @JS() abstract class X { - /*external String get 5abcde;*/ - /*external set 5abcde(String v);*/ + external String get JS$5abcde; + external set JS$5abcde(String v); external factory X(); }`); expectTranslate(`interface X { '_wxyz': string; }`).to.equal(`@anonymous @JS() abstract class X { - /*external String get _wxyz;*/ - /*external set _wxyz(String v);*/ + external String get JS$_wxyz; + external set JS$_wxyz(String v); external factory X(); }`); expectTranslate(`interface X { 'foo_34_81$': string; }`).to.equal(`@anonymous diff --git a/test/facade_converter_test.ts b/test/facade_converter_test.ts index eb10605..c304ae9 100644 --- a/test/facade_converter_test.ts +++ b/test/facade_converter_test.ts @@ -193,6 +193,65 @@ external set x(X v);`); }); describe('special identifiers', () => { + // For the Dart keyword list see + // https://dart.dev/guides/language/language-tour#keywords + it('always renames identifiers that are reserved keywords in Dart', () => { + expectTranslate(`declare var rethrow: number;`).to.equal(`@JS() +external num get JS$rethrow; +@JS() +external set JS$rethrow(num v);`); + + expectTranslate(`class X { while: string; }`).to.equal(`@JS() +class X { + // @Ignore + X.fakeConstructor$(); + external String get JS$while; + external set JS$while(String v); +}`); + }); + + it('only renames built-in keywords when they are used as class or type names', () => { + expectTranslate(`declare var abstract: number;`).to.equal(`@JS() +external num get abstract; +@JS() +external set abstract(num v);`); + + expectTranslate(`declare function get(): void;`).to.equal(`@JS() +external void get();`); + + expectTranslate(`interface X { abstract: string; }`).to.equal(`@anonymous +@JS() +abstract class X { + external String get abstract; + external set abstract(String v); + external factory X({String abstract}); +}`); + + expectTranslate(`interface X { get: number; }`).to.equal(`@anonymous +@JS() +abstract class X { + external num get get; + external set get(num v); + external factory X({num get}); +}`); + + expectTranslate(`interface abstract { a: number; }`).to.equal(`@anonymous +@JS() +abstract class JS$abstract { + external num get a; + external set a(num v); + external factory JS$abstract({num a}); +}`); + + expectTranslate(`class covariant { x: boolean; }`).to.equal(`@JS() +class JS$covariant { + // @Ignore + JS$covariant.fakeConstructor$(); + external bool get x; + external set x(bool v); +}`); + }); + it('preserves names that begin with two underscores', () => { expectWithTypes(`export function f(__a: number): boolean; export function f(__a: string): boolean;`)