Improved handling of invalid identifiers

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]);

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 = <ts.LiteralExpression>node;
-        this.emit(sLit.text);
-      } break;
       case ts.SyntaxKind.CallSignature: {
         let fn = <ts.SignatureDeclaration>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,

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<string> = 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<string> = 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<string> = new Set();
   private typingsRootRegex: RegExp;

lib/merge.ts

@@ -429,7 +429,7 @@ export function normalizeSourceFile(f: ts.SourceFile, fc: FacadeConverter, expli
       undefined {
     const members = classLike.members as ts.NodeArray<ts.ClassElement>;
     return members.find((member: ts.ClassElement) => {
-      if (base.ident(member.name) === propName) {
+      if (member.name && base.ident(member.name) === propName) {
         return true;
       }
     });

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('.');
   }

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;

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

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;`)