feat(jsii): emit warnings when using reserved words (#704)

* feat(jsii): emit warnings when using reserved words

Emits warnings when an exported API is named after a reserved word in
any of the supported target languages (best-effort). This warns users
that their code may have toruble compiling for certain target languages
and invites them to use a different name.

Additionally, a `--fail-on-warnings` / `--Werr` option was added to the
CLI that allows treating warnings as errors, allowing users to create
and maintain a codebase that does not rely on naming things in ways that
will cause conflicts in target languages.

Fixes #701

* also prohibit using 'build'

Author: RomainMuller

packages/jsii/bin/jsii.ts

@@ -22,6 +22,11 @@ import { VERSION } from '../lib/version';
             default: true,
             desc: 'Automatically add missing entries in the peerDependencies section of package.json'
         })
+        .options('fail-on-warnings', {
+            alias: 'Werr',
+            type: 'boolean',
+            desc: 'Treat warnings as errors'
+        })
         .help()
         .version(VERSION)
         .argv;
@@ -35,15 +40,16 @@ import { VERSION } from '../lib/version';
     const compiler = new Compiler({
         projectInfo,
         watch: argv.watch,
-        projectReferences: argv['project-references']
+        projectReferences: argv['project-references'],
+        failOnWarnings: argv['fail-on-warnings']
     });
 
     return { projectRoot, emitResult: await compiler.emit() };
 })().then(({ projectRoot, emitResult }) => {
     for (const diagnostic of emitResult.diagnostics) {
         utils.logDiagnostic(diagnostic, projectRoot);
     }
-    if (emitResult.hasErrors) {
+    if (emitResult.emitSkipped) {
         process.exit(1);
     }
 }).catch(e => {

packages/jsii/lib/assembler.ts

@@ -12,6 +12,7 @@ import { getReferencedDocParams, parseSymbolDocumentation } from './docs';
 import { Diagnostic, EmitResult, Emitter } from './emitter';
 import literate = require('./literate');
 import { ProjectInfo } from './project-info';
+import { isReservedName } from './reserved-words';
 import { Validator } from './validator';
 import { SHORT_VERSION, VERSION } from './version';
 
@@ -91,7 +92,7 @@ export class Assembler implements Emitter {
       // Clearing ``this._types`` to allow contents to be garbage-collected.
       delete this._types;
       try {
-        return { diagnostics: this._diagnostics, hasErrors: true };
+        return { diagnostics: this._diagnostics, emitSkipped: true };
       } finally {
         // Clearing ``this._diagnostics`` to allow contents to be garbage-collected.
         delete this._diagnostics;
@@ -124,7 +125,7 @@ export class Assembler implements Emitter {
 
     const validator = new Validator(this.projectInfo, assembly);
     const validationResult = await validator.emit();
-    if (!validationResult.hasErrors) {
+    if (!validationResult.emitSkipped) {
       const assemblyPath = path.join(this.projectInfo.projectRoot, '.jsii');
       LOG.trace(`Emitting assembly: ${colors.blue(assemblyPath)}`);
       await fs.writeJson(assemblyPath, _fingerprint(assembly), { encoding: 'utf8', spaces: 2 });
@@ -133,7 +134,7 @@ export class Assembler implements Emitter {
     try {
       return {
         diagnostics: [...this._diagnostics, ...validationResult.diagnostics],
-        hasErrors: validationResult.hasErrors
+        emitSkipped: validationResult.emitSkipped
       };
     } finally {
       // Clearing ``this._types`` to allow contents to be garbage-collected.
@@ -439,6 +440,8 @@ export class Assembler implements Emitter {
       return undefined;
     }
 
+    this._warnAboutReservedWords(type.symbol);
+
     const fqn = `${[this.projectInfo.name, ...ctx.namespace].join('.')}.${type.symbol.name}`;
 
     const jsiiType: spec.ClassType = {
@@ -747,6 +750,8 @@ export class Assembler implements Emitter {
       return undefined;
     }
 
+    this._warnAboutReservedWords(type.symbol);
+
     const decl = symbol.valueDeclaration;
     const flags = ts.getCombinedModifierFlags(decl);
     // tslint:disable-next-line:no-bitwise
@@ -822,6 +827,8 @@ export class Assembler implements Emitter {
       return undefined;
     }
 
+    this._warnAboutReservedWords(type.symbol);
+
     const fqn = `${[this.projectInfo.name, ...ctx.namespace].join('.')}.${type.symbol.name}`;
 
     const jsiiType: spec.InterfaceType = {
@@ -954,6 +961,8 @@ export class Assembler implements Emitter {
       this._diagnostic(declaration, ts.DiagnosticCategory.Error, `Prohibited member name: ${symbol.name}`);
       return;
     }
+    this._warnAboutReservedWords(symbol);
+
     const parameters = await Promise.all(signature.getParameters().map(p => this._toParameter(p, ctx)));
 
     const returnType = signature.getReturnType();
@@ -1006,6 +1015,16 @@ export class Assembler implements Emitter {
     type.methods.push(method);
   }
 
+  private _warnAboutReservedWords(symbol: ts.Symbol) {
+    const reservingLanguages = isReservedName(symbol.name);
+    if (reservingLanguages) {
+      this._diagnostic(symbol.valueDeclaration,
+        ts.DiagnosticCategory.Warning,
+        `'${symbol.name}' is a reserved word in ${reservingLanguages.join(', ')}. Using this name may cause problems `
+        + 'when generating language bindings. Consider using a different name.');
+    }
+  }
+
   private async _visitProperty(symbol: ts.Symbol, type: spec.ClassType | spec.InterfaceType, ctx: EmitContext) {
     if (type.properties && type.properties.find(p => p.name === symbol.name)) {
       /*
@@ -1024,6 +1043,8 @@ export class Assembler implements Emitter {
       return;
     }
 
+    this._warnAboutReservedWords(symbol);
+
     const signature = symbol.valueDeclaration as (ts.PropertySignature
       | ts.PropertyDeclaration
       | ts.AccessorDeclaration
@@ -1069,6 +1090,8 @@ export class Assembler implements Emitter {
     }
     const paramDeclaration = paramSymbol.valueDeclaration as ts.ParameterDeclaration;
 
+    this._warnAboutReservedWords(paramSymbol);
+
     const parameter: spec.Parameter = {
       ...await this._optionalValue(this._typeChecker.getTypeAtLocation(paramSymbol.valueDeclaration), paramSymbol.valueDeclaration),
       name: paramSymbol.name,
@@ -1643,12 +1666,11 @@ function isErrorType(t: ts.Type) {
 }
 
 /**
- * These specifially cause trouble in C#, where we have to specificially annotate them as 'new' but our generator isn't doing that
- *
- * In C#, 'GetHashCode' is also problematic, but jsii already prevents you from naming a
- * method that starts with 'get' so we don't need to do anything special for that.
+ * Those have specific semantics in certain languages that don't always translate cleanly in others
+ * (like how equals/hashCode are not a thing in Javascript, but carry meaning in Java and C#). The
+ * `build` name is reserved for generated code (Java builders use that).
  */
-const PROHIBITED_MEMBER_NAMES = ['equals', 'hashcode'];
+const PROHIBITED_MEMBER_NAMES = ['build', 'equals', 'hashcode'];
 
 /**
  * Whether the given name is prohibited

packages/jsii/lib/compiler.ts

@@ -44,6 +44,8 @@ export interface CompilerOptions {
     watch?: boolean;
     /** Whether to detect and generate TypeScript project references */
     projectReferences?: boolean;
+    /** Whether to fail when a warning is emitted */
+    failOnWarnings?: boolean;
 }
 
 export interface TypescriptConfig {
@@ -147,7 +149,7 @@ export class Compiler implements Emitter {
 
     private async _consumeProgram(program: ts.Program, stdlib: string): Promise<EmitResult> {
         const emit = program.emit();
-        let hasErrors = emitHasErrors(emit);
+        let hasErrors = emitHasErrors(emit, this.options.failOnWarnings);
         const diagnostics = [...emit.diagnostics];
 
         if (hasErrors) {
@@ -160,17 +162,17 @@ export class Compiler implements Emitter {
         try {
             const assembler = new Assembler(this.options.projectInfo, program, stdlib);
             const assmEmit = await assembler.emit();
-            if (assmEmit.hasErrors) {
+            if (assmEmit.emitSkipped) {
                 LOG.error('Type model errors prevented the JSII assembly from being created');
             }
 
-            hasErrors = hasErrors || assmEmit.hasErrors;
+            hasErrors = hasErrors || emitHasErrors(assmEmit, this.options.failOnWarnings);
             diagnostics.push(...assmEmit.diagnostics);
         } catch (e) {
             LOG.error(`Error during type model analysis: ${e}`);
         }
 
-        return { hasErrors, diagnostics };
+        return { emitSkipped: hasErrors, diagnostics, emittedFiles: emit.emittedFiles };
     }
 
     /**
@@ -371,6 +373,9 @@ function parseConfigHostFromCompilerHost(host: ts.CompilerHost): ts.ParseConfigH
     };
 }
 
-function emitHasErrors(result: ts.EmitResult) {
-    return result.diagnostics.some(d => d.category === ts.DiagnosticCategory.Error) || result.emitSkipped;
+function emitHasErrors(result: ts.EmitResult, includeWarnings?: boolean) {
+    return result.diagnostics.some(d =>
+        d.category === ts.DiagnosticCategory.Error
+        || (includeWarnings && d.category === ts.DiagnosticCategory.Warning)
+    ) || result.emitSkipped;
 }

packages/jsii/lib/emitter.ts

@@ -15,10 +15,7 @@ export interface Emitter {
 /**
  * The result of attempting to emit stuff.
  */
-export interface EmitResult {
-    /** Whether the emit was skipped as a result of errors (found in ``diagnostics``) */
-    hasErrors: boolean;
-
+export interface EmitResult extends ts.EmitResult {
     /** Diagnostic information created when trying to emit stuff */
     diagnostics: ReadonlyArray<ts.Diagnostic | Diagnostic>;
 }