fix(rosetta): newlines after return statements missing (#3063)

Handling of newlines was incorrect (and missing a terminating
semicolon) around lines involving `return`s.

Fix that, and fix correct handling of derived function return
types while we're at it.

Fixes #3054.



---

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

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

@@ -20,10 +20,6 @@ export function determineJsiiType(typeChecker: ts.TypeChecker, type: ts.Type): J
 
   type = type.getNonNullableType();
 
-  if (type.isUnion() || type.isIntersection()) {
-    return { kind: 'error', message: 'Type unions or intersections are not supported in examples' };
-  }
-
   const mapValuesType = mapElementType(type, typeChecker);
   if (mapValuesType.result === 'map') {
     return {
@@ -43,9 +39,12 @@ export function determineJsiiType(typeChecker: ts.TypeChecker, type: ts.Type): J
   }
 
   const typeScriptBuiltInType = builtInTypeName(type);
-  if (!typeScriptBuiltInType) {
-    return { kind: 'unknown' };
+  if (typeScriptBuiltInType) {
+    return { kind: 'builtIn', builtIn: typeScriptBuiltInType };
   }
 
-  return { kind: 'builtIn', builtIn: typeScriptBuiltInType };
+  if (type.isUnion() || type.isIntersection()) {
+    return { kind: 'error', message: 'Type unions or intersections are not supported in examples' };
+  }
+  return { kind: 'unknown' };
 }

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

@@ -15,7 +15,12 @@ import {
   findEnclosingClassDeclaration,
 } from '../typescript/ast-utils';
 import { ImportStatement } from '../typescript/imports';
-import { typeContainsUndefined, parameterAcceptsUndefined, inferMapElementType } from '../typescript/types';
+import {
+  typeContainsUndefined,
+  parameterAcceptsUndefined,
+  inferMapElementType,
+  determineReturnType,
+} from '../typescript/types';
 import { flat, partition, setExtend } from '../util';
 import { DefaultVisitor } from './default';
 import { TargetLanguage } from './target-language';
@@ -176,14 +181,16 @@ export class CSharpVisitor extends DefaultVisitor<CSharpLanguageContext> {
 
   // tslint:disable-next-line:max-line-length
   public functionLike(
-    node: ts.FunctionLikeDeclarationBase,
+    node: ts.FunctionLikeDeclaration | ts.ConstructorDeclaration | ts.MethodDeclaration,
     renderer: CSharpRenderer,
     opts: { isConstructor?: boolean } = {},
   ): OTree {
     const methodName = opts.isConstructor
       ? findEnclosingClassDeclaration(node)?.name?.text ?? 'MyClass'
       : renderer.updateContext({ propertyOrMethod: true }).convert(node.name);
-    const returnType = opts.isConstructor ? '' : this.renderTypeNode(node.type, false, renderer);
+
+    const retType = determineReturnType(renderer.typeChecker, node);
+    const returnType = opts.isConstructor ? '' : this.renderType(node, retType, false, 'void', renderer);
 
     const baseConstructorCall = new Array<string | OTree>();
     if (opts.isConstructor) {

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

@@ -17,8 +17,10 @@ export abstract class DefaultVisitor<C> implements AstHandler<C> {
 
   public abstract mergeContext(old: C, update: C): C;
 
+  protected statementTerminator = ';';
+
   public commentRange(comment: CommentSyntax, _context: AstRenderer<C>): OTree {
-    return new OTree([comment.text, comment.hasTrailingNewLine ? '\n' : '']);
+    return new OTree([comment.isTrailing ? ' ' : '', comment.text, comment.hasTrailingNewLine ? '\n' : '']);
   }
 
   public sourceFile(node: ts.SourceFile, context: AstRenderer<C>): OTree {
@@ -58,7 +60,9 @@ export abstract class DefaultVisitor<C> implements AstHandler<C> {
   }
 
   public returnStatement(node: ts.ReturnStatement, children: AstRenderer<C>): OTree {
-    return new OTree(['return ', children.convert(node.expression)]);
+    return new OTree(['return ', children.convert(node.expression), this.statementTerminator], [], {
+      canBreakLine: true,
+    });
   }
 
   public binaryExpression(node: ts.BinaryExpression, context: AstRenderer<C>): OTree {

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

@@ -8,7 +8,7 @@ import { OTree, NO_SYNTAX } from '../o-tree';
 import { AstRenderer } from '../renderer';
 import { isReadOnly, matchAst, nodeOfType, quoteStringLiteral, visibility } from '../typescript/ast-utils';
 import { ImportStatement } from '../typescript/imports';
-import { isEnumAccess, isStaticReadonlyAccess } from '../typescript/types';
+import { isEnumAccess, isStaticReadonlyAccess, determineReturnType } from '../typescript/types';
 import { DefaultVisitor } from './default';
 
 interface JavaContext {
@@ -236,11 +236,13 @@ export class JavaVisitor extends DefaultVisitor<JavaContext> {
   }
 
   public methodDeclaration(node: ts.MethodDeclaration, renderer: JavaRenderer): OTree {
-    return this.renderProcedure(node, renderer, node.name, this.renderTypeNode(node.type, renderer, 'void'));
+    const retType = determineReturnType(renderer.typeChecker, node);
+    return this.renderProcedure(node, renderer, node.name, this.renderType(node, retType, renderer, 'void'));
   }
 
   public functionDeclaration(node: ts.FunctionDeclaration, renderer: JavaRenderer): OTree {
-    return this.renderProcedure(node, renderer, node.name, this.renderTypeNode(node.type, renderer, 'void'));
+    const retType = determineReturnType(renderer.typeChecker, node);
+    return this.renderProcedure(node, renderer, node.name, this.renderType(node, retType, renderer, 'void'));
   }
 
   public methodSignature(node: ts.MethodSignature, renderer: JavaRenderer): OTree {
@@ -660,7 +662,10 @@ export class JavaVisitor extends DefaultVisitor<JavaContext> {
     return this.renderType(typeNode, renderer.typeOfType(typeNode), renderer, fallback);
   }
 
-  private renderType(owningNode: ts.Node, type: ts.Type, renderer: JavaRenderer, fallback: string): string {
+  private renderType(owningNode: ts.Node, type: ts.Type | undefined, renderer: JavaRenderer, fallback: string): string {
+    if (!type) {
+      return fallback;
+    }
     return doRender(determineJsiiType(renderer.typeChecker, type), false);
 
     // eslint-disable-next-line consistent-return

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

@@ -86,6 +86,8 @@ export class PythonVisitor extends DefaultVisitor<PythonLanguageContext> {
   public readonly language = TargetLanguage.PYTHON;
   public readonly defaultContext = {};
 
+  protected statementTerminator = '';
+
   public constructor(private readonly options: PythonVisitorOptions = {}) {
     super();
   }
@@ -102,7 +104,7 @@ export class PythonVisitor extends DefaultVisitor<PythonLanguageContext> {
       .join('\n');
     const needsAdditionalTrailer = comment.hasTrailingNewLine;
 
-    return new OTree([hashLines, needsAdditionalTrailer ? '\n' : ''], [], {
+    return new OTree([comment.isTrailing ? ' ' : '', hashLines, needsAdditionalTrailer ? '\n' : ''], [], {
       // Make sure comment is rendered exactly once in the output tree, no
       // matter how many source nodes it is attached to.
       renderOnce: `comment-${comment.pos}`,

packages/jsii-rosetta/lib/renderer.ts

@@ -191,6 +191,23 @@ export class AstRenderer<C> {
     }
   }
 
+  /**
+   * Whether there is non-whitespace on the same line before the given position
+   */
+  public codeOnLineBefore(pos: number) {
+    const text = this.sourceFile.text;
+    while (pos > 0) {
+      const c = text[--pos];
+      if (c === '\n') {
+        return false;
+      }
+      if (c !== ' ' && c !== '\r' && c !== '\t') {
+        return true;
+      }
+    }
+    return false;
+  }
+
   /**
    * Return a newline if the given node is preceded by at least one newline
    *
@@ -561,6 +578,11 @@ export interface CommentSyntax {
   text: string;
   hasTrailingNewLine?: boolean;
   kind: ts.CommentKind;
+
+  /**
+   * Whether it's at the end of a code line (so we can render a separating space)
+   */
+  isTrailing?: boolean;
 }
 
 function commentSyntaxFromCommentRange(rng: ts.CommentRange, renderer: AstRenderer<any>): CommentSyntax {
@@ -569,5 +591,6 @@ function commentSyntaxFromCommentRange(rng: ts.CommentRange, renderer: AstRender
     kind: rng.kind,
     pos: rng.pos,
     text: renderer.textAt(rng.pos, rng.end),
+    isTrailing: renderer.codeOnLineBefore(rng.pos),
   };
 }

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

@@ -17,17 +17,19 @@ export function firstTypeInUnion(typeChecker: ts.TypeChecker, type: ts.Type): ts
 
 export type BuiltInType = 'any' | 'boolean' | 'number' | 'string';
 export function builtInTypeName(type: ts.Type): BuiltInType | undefined {
-  const map: { readonly [k: number]: BuiltInType } = {
-    [ts.TypeFlags.Any]: 'any',
-    [ts.TypeFlags.Unknown]: 'any',
-    [ts.TypeFlags.Boolean]: 'boolean',
-    [ts.TypeFlags.Number]: 'number',
-    [ts.TypeFlags.String]: 'string',
-    [ts.TypeFlags.StringLiteral]: 'string',
-    [ts.TypeFlags.NumberLiteral]: 'number',
-    [ts.TypeFlags.BooleanLiteral]: 'boolean',
-  };
-  return map[type.flags];
+  if (hasAnyFlag(type.flags, ts.TypeFlags.Any | ts.TypeFlags.Unknown)) {
+    return 'any';
+  }
+  if (hasAnyFlag(type.flags, ts.TypeFlags.BooleanLike)) {
+    return 'boolean';
+  }
+  if (hasAnyFlag(type.flags, ts.TypeFlags.NumberLike)) {
+    return 'number';
+  }
+  if (hasAnyFlag(type.flags, ts.TypeFlags.StringLike)) {
+    return 'string';
+  }
+  return undefined;
 }
 
 export function renderType(type: ts.Type): string {
@@ -184,3 +186,11 @@ export function renderFlags(flags: number | undefined, flagObject: Record<string
     .map((f) => flagObject[f])
     .join(',');
 }
+
+export function determineReturnType(typeChecker: ts.TypeChecker, node: ts.SignatureDeclaration): ts.Type | undefined {
+  const signature = typeChecker.getSignatureFromDeclaration(node);
+  if (!signature) {
+    return undefined;
+  }
+  return typeChecker.getReturnTypeOfSignature(signature);
+}

packages/jsii-rosetta/test/translations/statements/statements_and_newlines.cs

@@ -0,0 +1,26 @@
+public int DoThing()
+{
+    int x = 1; // x seems to be equal to 1
+    return x + 1;
+}
+
+public boolean DoThing2(int x)
+{
+    if (x == 1)
+    {
+        return true;
+    }
+    return false;
+}
+
+public int DoThing3()
+{
+    int x = 1;
+    return x + 1;
+}
+
+public void DoThing4()
+{
+    int x = 1;
+    x = 85;
+}

packages/jsii-rosetta/test/translations/statements/statements_and_newlines.java

@@ -0,0 +1,21 @@
+public Number doThing() {
+    Number x = 1; // x seems to be equal to 1
+    return x + 1;
+}
+
+public boolean doThing2(Number x) {
+    if (x == 1) {
+        return true;
+    }
+    return false;
+}
+
+public Number doThing3() {
+    Number x = 1;
+    return x + 1;
+}
+
+public void doThing4() {
+    Number x = 1;
+    x = 85;
+}

packages/jsii-rosetta/test/translations/statements/statements_and_newlines.py

@@ -0,0 +1,16 @@
+def do_thing():
+    x = 1 # x seems to be equal to 1
+    return x + 1
+
+def do_thing2(x):
+    if x == 1:
+        return True
+    return False
+
+def do_thing3():
+    x = 1
+    return x + 1
+
+def do_thing4():
+    x = 1
+    x = 85

packages/jsii-rosetta/test/translations/statements/statements_and_newlines.ts

@@ -0,0 +1,21 @@
+function doThing() {
+  const x = 1; // x seems to be equal to 1
+  return x + 1;
+}
+
+function doThing2(x: number) {
+  if (x == 1) {
+    return true;
+  }
+  return false;
+}
+
+function doThing3() {
+  const x = 1;
+  return x + 1;
+}
+
+function doThing4() {
+  let x = 1;
+  x = 85;
+}