Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Oct 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 6 additions & 7 deletions packages/jsii-rosetta/lib/jsii/jsii-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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' };
}
13 changes: 10 additions & 3 deletions packages/jsii-rosetta/lib/languages/csharp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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) {
Expand Down
8 changes: 6 additions & 2 deletions packages/jsii-rosetta/lib/languages/default.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
13 changes: 9 additions & 4 deletions packages/jsii-rosetta/lib/languages/java.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion packages/jsii-rosetta/lib/languages/python.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand All @@ -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}`,
Expand Down
23 changes: 23 additions & 0 deletions packages/jsii-rosetta/lib/renderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand Down Expand Up @@ -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 {
Expand All @@ -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),
};
}
32 changes: 21 additions & 11 deletions packages/jsii-rosetta/lib/typescript/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
}
Original file line number Diff line number Diff line change
@@ -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;
}
Original file line number Diff line number Diff line change
@@ -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;
}
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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;
}