fix(rosetta): fix usage of Builders in Java (#3058)

Fixes two issues:

## Builders were never used for structs

It's required to use builders to instantiate structs -- the code we generated would generate:

```java
new MyStruct().setter(1).setter(2)
```

But the actual required syntax is 

```java
MyStruct.builder().setter(1).setter(2).build();
```

This was caused because there's a confusion: the structs jsii generates have a builder. *However*, we also support declaring classes and structs *inline* in the example, and to not muck up the example too much we don't generate builders for those.

Since it's probably desirable to show the definition of some constructs (like stacks) and some props types in examples, we can't get rid of that capability. So instead, check if the struct/class we're referencing actually comes from jsii or not, and use a builder if it does.

## Classes never use the combined class+struct builder

Classes with a struct argument can be instantiated in two ways:

```java
new MyClass(1, 2, MyStruct.builder()
    .setter(1)
    .setter(2))
    .build());
```

As well as:

```
MyClass.Builder.create(1, 2)
    .setter(1)
    .setter(2)
    .build();
```

Prefer the second style as it's less noisy.


Relates to #2984.

This PR also switches off the `consistent-return` eslint warning, as it hurts more than helps when we have TypeScript available.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license].

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0

eslint-config.yaml

@@ -138,9 +138,6 @@ rules:
   'complexity':
     - off
 
-  'consistent-return':
-    - error
-
   'dot-notation':
     - error
 
@@ -240,3 +237,9 @@ rules:
   '@typescript-eslint/unbound-method': off
   'no-case-declarations': off
   'require-atomic-updates': off
+
+  # 'consistent-return' actually decreases safety. Its use will enforce useless `throws`
+  # statements, forcing a runtime error that occlude cases where the TypeScript type
+  # checker would actually have caught something like a non-exhaustive `switch` statement
+  # at compile time.
+  'consistent-return': off

packages/jsii-rosetta/lib/jsii/jsii-types.ts

@@ -1,6 +1,7 @@
 import * as ts from 'typescript';
 
-import { BuiltInType, builtInTypeName, mapElementType } from '../typescript/types';
+import { inferredTypeOfExpression, BuiltInType, builtInTypeName, mapElementType } from '../typescript/types';
+import { hasAnyFlag, analyzeStructType } from './jsii-utils';
 
 // eslint-disable-next-line prettier/prettier
 export type JsiiType =
@@ -48,3 +49,49 @@ export function determineJsiiType(typeChecker: ts.TypeChecker, type: ts.Type): J
   }
   return { kind: 'unknown' };
 }
+
+export type ObjectLiteralAnalysis =
+  | { readonly kind: 'struct'; readonly type: ts.Type }
+  | { readonly kind: 'local-struct'; readonly type: ts.Type }
+  | { readonly kind: 'map' }
+  | { readonly kind: 'unknown' };
+
+export function analyzeObjectLiteral(
+  typeChecker: ts.TypeChecker,
+  node: ts.ObjectLiteralExpression,
+): ObjectLiteralAnalysis {
+  const type = inferredTypeOfExpression(typeChecker, node);
+  if (!type) {
+    return { kind: 'unknown' };
+  }
+
+  const call = findEnclosingCallExpression(node);
+  const isDeclaredCall = !!(call && typeChecker.getResolvedSignature(call)?.declaration);
+
+  if (hasAnyFlag(type.flags, ts.TypeFlags.Any)) {
+    // The type checker by itself won't tell us the difference between an `any` that
+    // was literally declared as a type in the code, vs an `any` it assumes because it
+    // can't find a function's type declaration.
+    //
+    // Search for the function's declaration and only if we can't find it,
+    // the type is actually unknown (otherwise it's a literal 'any').
+    return isDeclaredCall ? { kind: 'map' } : { kind: 'unknown' };
+  }
+
+  const structType = analyzeStructType(type);
+  if (structType) {
+    return { kind: structType, type };
+  }
+  return { kind: 'map' };
+}
+
+function findEnclosingCallExpression(node?: ts.Node): ts.CallLikeExpression | undefined {
+  while (node) {
+    if (ts.isCallLikeExpression(node)) {
+      return node;
+    }
+    node = node.parent;
+  }
+
+  return undefined;
+}

packages/jsii-rosetta/lib/jsii/jsii-utils.ts

@@ -2,18 +2,27 @@ import * as ts from 'typescript';
 
 import { AstRenderer } from '../renderer';
 import { typeContainsUndefined } from '../typescript/types';
+import { findPackageJson } from './packages';
 
-export function isStructInterface(name: string) {
+export function isNamedLikeStruct(name: string) {
   // Start with an I and another uppercase character
   return !/^I[A-Z]/.test(name);
 }
 
-export function isStructType(type: ts.Type) {
-  return (
-    type.isClassOrInterface() &&
-    hasAllFlags(type.objectFlags, ts.ObjectFlags.Interface) &&
-    isStructInterface(type.symbol.name)
-  );
+export function analyzeStructType(type: ts.Type): 'struct' | 'local-struct' | false {
+  if (
+    !type.isClassOrInterface() ||
+    !hasAllFlags(type.objectFlags, ts.ObjectFlags.Interface) ||
+    !isNamedLikeStruct(type.symbol.name)
+  ) {
+    return false;
+  }
+
+  if (refersToJsiiSymbol(type.symbol)) {
+    return 'struct';
+  }
+
+  return 'local-struct';
 }
 
 export function hasAllFlags<A extends number>(flags: A, test: A) {
@@ -57,3 +66,32 @@ export function propertiesOfStruct(type: ts.Type, context: AstRenderer<any>): St
 export function structPropertyAcceptsUndefined(prop: StructProperty): boolean {
   return prop.questionMark || (!!prop.type && typeContainsUndefined(prop.type));
 }
+
+/**
+ * Whether or not the given call expression seems to refer to a jsii symbol
+ *
+ * If it does, we treat it differently than if it's a class or symbol defined
+ * in the same example source.
+ *
+ * To do this, we look for whether it's defined in a directory that's compiled
+ * for jsii and has a jsii assembly.
+ *
+ * FIXME: Look up the actual symbol identifier when we finally have those.
+ *
+ * For tests, we also treat symbols in a file that has the string '/// fake-from-jsii'
+ * as coming from jsii.
+ */
+export function refersToJsiiSymbol(symbol: ts.Symbol): boolean {
+  const declaration = symbol.declarations[0];
+  if (!declaration) {
+    return false;
+  }
+
+  const declaringFile = declaration.getSourceFile();
+  if (/^\/\/\/ fake-from-jsii/m.test(declaringFile.getFullText())) {
+    return true;
+  }
+
+  const pj = findPackageJson(declaringFile.fileName);
+  return !!(pj && pj.jsii);
+}

packages/jsii-rosetta/lib/jsii/packages.ts

@@ -1,3 +1,6 @@
+import * as fs from 'fs';
+import * as path from 'path';
+
 /**
  * Resolve a package name in an example to a JSII assembly
  *
@@ -15,6 +18,28 @@ export function resolvePackage(packageName: string) {
   }
 }
 
+/**
+ * Find an enclosing package.json file given a filename
+ *
+ * Will return `undefined` if a package.json could not be found.
+ */
+export function findPackageJson(fileName: string) {
+  // eslint-disable-next-line no-constant-condition
+  while (true) {
+    const candidatePath = path.join(fileName, 'package.json');
+    if (fs.existsSync(candidatePath)) {
+      // eslint-disable-next-line @typescript-eslint/no-require-imports
+      return require(path.resolve(candidatePath));
+    }
+
+    const parent = path.dirname(fileName);
+    if (parent === fileName) {
+      return undefined;
+    }
+    fileName = parent;
+  }
+}
+
 export function jsiiTargetParam(packageName: string, field: string) {
   const pkgJson = resolvePackage(packageName);
 

packages/jsii-rosetta/lib/languages/csharp.ts

@@ -418,6 +418,7 @@ export class CSharpVisitor extends DefaultVisitor<CSharpLanguageContext> {
   public knownStructObjectLiteralExpression(
     node: ts.ObjectLiteralExpression,
     structType: ts.Type,
+    _definedInExample: boolean,
     renderer: CSharpRenderer,
   ): OTree {
     return new OTree(['new ', structType.symbol.name, ' { '], renderer.convertAll(node.properties), {

packages/jsii-rosetta/lib/languages/default.ts

@@ -1,6 +1,7 @@
 import * as ts from 'typescript';
 
-import { isStructInterface, isStructType, hasAllFlags } from '../jsii/jsii-utils';
+import { analyzeObjectLiteral } from '../jsii/jsii-types';
+import { isNamedLikeStruct } from '../jsii/jsii-utils';
 import { OTree, NO_SYNTAX } from '../o-tree';
 import { AstRenderer, AstHandler, nimpl, CommentSyntax } from '../renderer';
 import { voidExpressionString } from '../typescript/ast-utils';
@@ -152,33 +153,17 @@ export abstract class DefaultVisitor<C> implements AstHandler<C> {
       context.report(unsup, `Use of ${ts.SyntaxKind[unsup.kind]} in an object literal is not supported.`);
     }
 
-    const type = context.inferredTypeOfExpression(node);
-
-    let isUnknownType = !type;
-    if (type && hasAllFlags(type.flags, ts.TypeFlags.Any)) {
-      // The type checker by itself won't tell us the difference between an `any` that
-      // was literally declared as a type in the code, vs an `any` it assumes because it
-      // can't find a function's type declaration.
-      //
-      // Search for the function's declaration and only if we can't find it,
-      // the type is actually unknown (otherwise it's a literal 'any').
-      const call = findEnclosingCallExpression(node);
-      const signature = call ? context.typeChecker.getResolvedSignature(call) : undefined;
-      if (!signature?.declaration) {
-        isUnknownType = true;
-      }
-    }
-
-    const isKnownStruct = type && isStructType(type);
+    const lit = analyzeObjectLiteral(context.typeChecker, node);
 
-    if (isUnknownType) {
-      return this.unknownTypeObjectLiteralExpression(node, context);
+    switch (lit.kind) {
+      case 'unknown':
+        return this.unknownTypeObjectLiteralExpression(node, context);
+      case 'struct':
+      case 'local-struct':
+        return this.knownStructObjectLiteralExpression(node, lit.type, lit.kind === 'local-struct', context);
+      case 'map':
+        return this.keyValueObjectLiteralExpression(node, context);
     }
-    if (isKnownStruct) {
-      // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion
-      return this.knownStructObjectLiteralExpression(node, type!, context);
-    }
-    return this.keyValueObjectLiteralExpression(node, context);
   }
 
   public unknownTypeObjectLiteralExpression(node: ts.ObjectLiteralExpression, context: AstRenderer<C>): OTree {
@@ -188,6 +173,7 @@ export abstract class DefaultVisitor<C> implements AstHandler<C> {
   public knownStructObjectLiteralExpression(
     node: ts.ObjectLiteralExpression,
     _structType: ts.Type,
+    _definedInExample: boolean,
     context: AstRenderer<C>,
   ): OTree {
     return this.notImplemented(node, context);
@@ -259,7 +245,7 @@ export abstract class DefaultVisitor<C> implements AstHandler<C> {
   }
 
   public interfaceDeclaration(node: ts.InterfaceDeclaration, context: AstRenderer<C>): OTree {
-    if (isStructInterface(context.textOf(node.name))) {
+    if (isNamedLikeStruct(context.textOf(node.name))) {
       return this.structInterfaceDeclaration(node, context);
     }
     return this.regularInterfaceDeclaration(node, context);
@@ -350,14 +336,3 @@ const UNARY_OPS: { [op in ts.PrefixUnaryOperator]: string } = {
   [ts.SyntaxKind.TildeToken]: '~',
   [ts.SyntaxKind.ExclamationToken]: '!',
 };
-
-function findEnclosingCallExpression(node?: ts.Node): ts.CallLikeExpression | undefined {
-  while (node) {
-    if (ts.isCallLikeExpression(node)) {
-      return node;
-    }
-    node = node.parent;
-  }
-
-  return undefined;
-}

packages/jsii-rosetta/lib/languages/java.ts

@@ -1,7 +1,6 @@
 import * as ts from 'typescript';
 
-import { determineJsiiType, JsiiType } from '../jsii/jsii-types';
-import { isStructType } from '../jsii/jsii-utils';
+import { determineJsiiType, JsiiType, analyzeObjectLiteral } from '../jsii/jsii-types';
 import { jsiiTargetParam } from '../jsii/packages';
 import { TargetLanguage } from '../languages/target-language';
 import { OTree, NO_SYNTAX } from '../o-tree';
@@ -417,38 +416,45 @@ export class JavaVisitor extends DefaultVisitor<JavaContext> {
   public newExpression(node: ts.NewExpression, renderer: JavaRenderer): OTree {
     const argsLength = node.arguments ? node.arguments.length : 0;
     const lastArg = argsLength > 0 ? node.arguments![argsLength - 1] : undefined;
-    const lastArgIsObjectLiteral = lastArg && ts.isObjectLiteralExpression(lastArg);
-    const lastArgType = lastArg && renderer.inferredTypeOfExpression(lastArg);
-    // we only render the ClassName.Builder.create(...) expression
-    // if the last argument is an object literal, and NOT a known struct
-    // (in that case, it has its own creation method)
-    const renderBuilderInsteadOfNew = lastArgIsObjectLiteral && (!lastArgType || !isStructType(lastArgType));
 
-    return new OTree(
-      [],
-      [
-        renderBuilderInsteadOfNew ? undefined : 'new ',
-        renderer
-          .updateContext({
-            ignorePropertyPrefix: true,
-            convertPropertyToGetter: false,
-          })
-          .convert(node.expression),
-        renderBuilderInsteadOfNew ? '.Builder.create' : undefined,
-        '(',
-        this.argumentList(
-          renderBuilderInsteadOfNew ? node.arguments!.slice(0, argsLength - 1) : node.arguments,
-          renderer,
-        ),
-        ')',
-        renderBuilderInsteadOfNew
-          ? renderer.updateContext({ inNewExprWithObjectLiteralAsLastArg: true }).convert(lastArg)
-          : undefined,
-      ],
-      {
-        canBreakLine: true,
-      },
-    );
+    // We render the ClassName.Builder.create(...) expression
+    // if the last argument is an object literal, and either a known struct (because
+    // those are the jsii rules) or an unknown type (because the example didn't
+    // compile but most of the target examples will intend this to be a struct).
+
+    const structArgument = lastArg && ts.isObjectLiteralExpression(lastArg) ? lastArg : undefined;
+    let renderBuilder = false;
+    if (lastArg && ts.isObjectLiteralExpression(lastArg)) {
+      const analysis = analyzeObjectLiteral(renderer.typeChecker, lastArg);
+      renderBuilder = analysis.kind === 'struct' || analysis.kind === 'unknown';
+    }
+
+    const className = renderer
+      .updateContext({
+        ignorePropertyPrefix: true,
+        convertPropertyToGetter: false,
+      })
+      .convert(node.expression);
+
+    if (renderBuilder) {
+      const initialArguments = node.arguments!.slice(0, argsLength - 1);
+      return new OTree(
+        [],
+        [
+          className,
+          '.Builder.create(',
+          this.argumentList(initialArguments, renderer),
+          ')',
+          ...renderer.convertAll(structArgument!.properties),
+          new OTree([renderer.mirrorNewlineBefore(structArgument!.properties[0])], ['.build()']),
+        ],
+        { canBreakLine: true, indent: 8 },
+      );
+    }
+
+    return new OTree([], ['new ', className, '(', this.argumentList(node.arguments, renderer), ')'], {
+      canBreakLine: true,
+    });
   }
 
   public unknownTypeObjectLiteralExpression(node: ts.ObjectLiteralExpression, renderer: JavaRenderer): OTree {
@@ -468,11 +474,30 @@ export class JavaVisitor extends DefaultVisitor<JavaContext> {
   public knownStructObjectLiteralExpression(
     node: ts.ObjectLiteralExpression,
     structType: ts.Type,
+    definedInExample: boolean,
     renderer: JavaRenderer,
   ): OTree {
-    return new OTree(['new ', structType.symbol.name, '()'], [...renderer.convertAll(node.properties)], {
-      indent: 8,
-    });
+    // Special case: we're rendering an object literal, but the containing constructor
+    // has already started the builder: we don't have to create a new instance,
+    // all we have to do is pile on arguments.
+    if (renderer.currentContext.inNewExprWithObjectLiteralAsLastArg) {
+      return new OTree([], renderer.convertAll(node.properties), { indent: 8 });
+    }
+
+    // Jsii-generated classes have builders, classes we generated in the course of
+    // this example do not.
+    const hasBuilder = !definedInExample;
+
+    return new OTree(
+      hasBuilder ? [structType.symbol.name, '.builder()'] : ['new ', structType.symbol.name, '()'],
+      [
+        ...renderer.convertAll(node.properties),
+        new OTree([renderer.mirrorNewlineBefore(node.properties[0])], [hasBuilder ? '.build()' : '']),
+      ],
+      {
+        indent: 8,
+      },
+    );
   }
 
   public propertyAssignment(node: ts.PropertyAssignment, renderer: JavaRenderer): OTree {