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): constants are incorrectly turned into getters #3050

Merged
merged 2 commits into from
Oct 11, 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
11 changes: 8 additions & 3 deletions packages/jsii-rosetta/lib/jsii/jsii-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,19 @@ export function isStructInterface(name: string) {
export function isStructType(type: ts.Type) {
return (
type.isClassOrInterface() &&
hasFlag(type.objectFlags, ts.ObjectFlags.Interface) &&
hasAllFlags(type.objectFlags, ts.ObjectFlags.Interface) &&
isStructInterface(type.symbol.name)
);
}

export function hasFlag<A extends number>(flags: A, test: A) {
export function hasAllFlags<A extends number>(flags: A, test: A) {
// tslint:disable-next-line:no-bitwise
return (flags & test) !== 0;
return test !== 0 && (flags & test) === test;
}

export function hasAnyFlag<A extends number>(flags: A, test: A) {
// tslint:disable-next-line:no-bitwise
return test !== 0 && (flags & test) !== 0;
}

export interface StructProperty {
Expand Down
4 changes: 2 additions & 2 deletions packages/jsii-rosetta/lib/languages/default.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as ts from 'typescript';

import { hasFlag, isStructInterface, isStructType } from '../jsii/jsii-utils';
import { isStructInterface, isStructType, hasAllFlags } from '../jsii/jsii-utils';
import { OTree, NO_SYNTAX } from '../o-tree';
import { AstRenderer, AstHandler, nimpl, CommentSyntax } from '../renderer';
import { voidExpressionString } from '../typescript/ast-utils';
Expand Down Expand Up @@ -133,7 +133,7 @@ export abstract class DefaultVisitor<C> implements AstHandler<C> {
const type = context.inferredTypeOfExpression(node);

let isUnknownType = !type;
if (type && hasFlag(type.flags, ts.TypeFlags.Any)) {
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.
Expand Down
24 changes: 19 additions & 5 deletions packages/jsii-rosetta/lib/languages/java.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,13 @@ 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 { builtInTypeName, firstTypeInUnion, mapElementType } from '../typescript/types';
import {
builtInTypeName,
mapElementType,
isEnumAccess,
isStaticReadonlyAccess,
firstTypeInUnion,
} from '../typescript/types';
import { DefaultVisitor } from './default';

interface JavaContext {
Expand Down Expand Up @@ -496,11 +502,19 @@ export class JavaVisitor extends DefaultVisitor<JavaContext> {
// for 'this', assume this is a field, and access it directly
parts = ['this', '.', rightHandSide];
} else {
let convertToGetter = renderer.currentContext.convertPropertyToGetter !== false;

// See if we're not accessing an enum member or public static readonly property (const).
if (isEnumAccess(renderer.typeChecker, node)) {
convertToGetter = false;
}
if (isStaticReadonlyAccess(renderer.typeChecker, node)) {
convertToGetter = false;
}

// add a 'get' prefix to the property name, and change the access to a method call, if required
const renderedRightHandSide =
renderer.currentContext.convertPropertyToGetter === false
? rightHandSide
: `get${capitalize(node.name.text)}()`;
const renderedRightHandSide = convertToGetter ? `get${capitalize(node.name.text)}()` : rightHandSide;

// strip any trailing ! from the left-hand side, as they're not meaningful in Java
parts = [stripTrailingBang(leftHandSide), '.', renderedRightHandSide];
}
Expand Down
46 changes: 36 additions & 10 deletions packages/jsii-rosetta/lib/typescript/types.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as ts from 'typescript';

import { hasAllFlags, hasAnyFlag } from '../jsii/jsii-utils';
import { AstRenderer } from '../renderer';

/**
Expand Down Expand Up @@ -41,16 +42,6 @@ export function renderType(type: ts.Type): string {
return renderTypeFlags(type);
}

export function renderTypeFlags(type: ts.Type) {
const ret = [];
for (const flag of Object.values(ts.TypeFlags)) {
if (typeof flag === 'number' && type.flags & flag) {
ret.push(ts.TypeFlags[flag]);
}
}
return ret.join(',');
}

export function parameterAcceptsUndefined(param: ts.ParameterDeclaration, type?: ts.Type): boolean {
if (param.initializer !== undefined) {
return true;
Expand All @@ -77,6 +68,10 @@ export function typeContainsUndefined(type: ts.Type): boolean {
return false;
}

export function renderTypeFlags(type: ts.Type): string {
return renderFlags(type.flags, ts.TypeFlags);
}

/**
* If this is a map type, return the type mapped *to* (key must always be `string` anyway).
*/
Expand Down Expand Up @@ -153,3 +148,34 @@ export function arrayElementType(type: ts.Type): ts.Type | undefined {
function isDefined<A>(x: A): x is NonNullable<A> {
return x !== undefined;
}

export function isNumber(x: any): x is number {
return typeof x === 'number';
}

export function isEnumAccess(typeChecker: ts.TypeChecker, access: ts.PropertyAccessExpression) {
const symbol = typeChecker.getSymbolAtLocation(access.expression);
return symbol ? hasAnyFlag(symbol.flags, ts.SymbolFlags.Enum) : false;
}

export function isStaticReadonlyAccess(typeChecker: ts.TypeChecker, access: ts.PropertyAccessExpression) {
const symbol = typeChecker.getSymbolAtLocation(access);
const decl = symbol?.getDeclarations();
if (decl && decl[0] && ts.isPropertyDeclaration(decl[0])) {
const flags = ts.getCombinedModifierFlags(decl[0]);
return hasAllFlags(flags, ts.ModifierFlags.Readonly | ts.ModifierFlags.Static);
}
return false;
}

export function renderFlags(flags: number | undefined, flagObject: Record<string, number | string>) {
if (flags === undefined) {
return '';
}

return Object.values(flagObject)
.filter(isNumber)
.filter((f) => hasAllFlags(flags, f))
.map((f) => flagObject[f])
.join(',');
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Console.WriteLine(EnumType.ENUM_VALUE_A);
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
System.out.println(EnumType.ENUM_VALUE_A);
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
print(EnumType.ENUM_VALUE_A)
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/// !hide
enum EnumType {
ENUM_VALUE_A = 'a',
ENUM_VALUE_B = 'b',
}
/// !show
console.log(EnumType.ENUM_VALUE_A);
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Console.WriteLine(EnumType.ENUM_VALUE_A);
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
System.out.println(EnumType.ENUM_VALUE_A);
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
print(EnumType.ENUM_VALUE_A)
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/// !hide
class EnumType {
public static readonly ENUM_VALUE_A = 'a';
public static readonly ENUM_VALUE_B = 'b';
}
/// !show
console.log(EnumType.ENUM_VALUE_A);