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.

Sign up for GitHub

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

feat(java): detect & rename members named after reserved words #705

Merged
merged 4 commits into from
Aug 14, 2019
Merged

feat(java): detect & rename members named after reserved words #705

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
12b4213
Java bindings: added automatic check and modification of reserved wor…
bmaizels Aug 13, 2019
7e8cc70
CR feedback: hashcode() implementation is no longer broken if there i…
bmaizels Aug 13, 2019
7fab611
Fix for equals() shadowing a property called 'that'
bmaizels Aug 13, 2019
94f15c6
Merge branch 'master' into bmaizels/jsii-java-reserved-words
RomainMuller Aug 14, 2019
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
17 changes: 17 additions & 0 deletions packages/jsii-calc/lib/compliance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1791,6 +1791,23 @@ export interface DiamondInheritanceTopLevelStruct extends DiamondInheritanceFirs
readonly topLevelProperty: string;
}

export interface StructWithJavaReservedWords {
readonly default: string;
readonly assert?: string;
}

export class ClassWithJavaReservedWords {
readonly int: string;

public constructor(int: string) {
this.int = int;
}

public import(assert: string): string {
return this.int + assert;
}
}

/**
* Just because we can.
*
Expand Down
122 changes: 118 additions & 4 deletions packages/jsii-calc/test/assembly.jsii
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1742,6 +1742,73 @@
},
"name": "ClassWithDocs"
},
"jsii-calc.ClassWithJavaReservedWords": {
"assembly": "jsii-calc",
"docs": {
"stability": "experimental"
},
"fqn": "jsii-calc.ClassWithJavaReservedWords",
"initializer": {
"docs": {
"stability": "experimental"
},
"parameters": [
{
"name": "int",
"type": {
"primitive": "string"
}
}
]
},
"kind": "class",
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 1799
},
"methods": [
{
"docs": {
"stability": "experimental"
},
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 1806
},
"name": "import",
"parameters": [
{
"name": "assert",
"type": {
"primitive": "string"
}
}
],
"returns": {
"type": {
"primitive": "string"
}
}
}
],
"name": "ClassWithJavaReservedWords",
"properties": [
{
"docs": {
"stability": "experimental"
},
"immutable": true,
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 1800
},
"name": "int",
"type": {
"primitive": "string"
}
}
]
},
"jsii-calc.ClassWithMutableObjectLiteralProperty": {
"assembly": "jsii-calc",
"docs": {
Expand Down Expand Up @@ -7978,7 +8045,7 @@
"kind": "class",
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 1799
"line": 1816
},
"methods": [
{
Expand All @@ -7987,7 +8054,7 @@
},
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 1808
"line": 1825
},
"name": "howManyVarArgsDidIPass",
"parameters": [
Expand Down Expand Up @@ -8019,7 +8086,7 @@
},
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 1800
"line": 1817
},
"name": "roundTrip",
"parameters": [
Expand All @@ -8046,6 +8113,53 @@
],
"name": "StructPassing"
},
"jsii-calc.StructWithJavaReservedWords": {
"assembly": "jsii-calc",
"datatype": true,
"docs": {
"stability": "experimental"
},
"fqn": "jsii-calc.StructWithJavaReservedWords",
"kind": "interface",
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 1794
},
"name": "StructWithJavaReservedWords",
"properties": [
{
"abstract": true,
"docs": {
"stability": "experimental"
},
"immutable": true,
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 1795
},
"name": "default",
"type": {
"primitive": "string"
}
},
{
"abstract": true,
"docs": {
"stability": "experimental"
},
"immutable": true,
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 1796
},
"name": "assert",
"optional": true,
"type": {
"primitive": "string"
}
}
]
},
"jsii-calc.Sum": {
"assembly": "jsii-calc",
"base": "jsii-calc.composition.CompositeOperation",
Expand Down Expand Up @@ -9215,5 +9329,5 @@
}
},
"version": "0.15.0",
"fingerprint": "SqE3g3zYpTXR3veJ0hoo+OuH7uDYav7rVxMrKNh88I4="
"fingerprint": "FjdHcguEQohvkGk8SSyRgDb8An5sYAqbTMYyGprjtiQ="
}
28 changes: 25 additions & 3 deletions ...-java-runtime-test/project/src/test/java/software/amazon/jsii/testing/ComplianceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import software.amazon.jsii.tests.calculator.AsyncVirtualMethods;
import software.amazon.jsii.tests.calculator.Calculator;
import software.amazon.jsii.tests.calculator.CalculatorProps;
import software.amazon.jsii.tests.calculator.ClassWithJavaReservedWords;
import software.amazon.jsii.tests.calculator.ClassWithPrivateConstructorAndAutomaticProperties;
import software.amazon.jsii.tests.calculator.Constructors;
import software.amazon.jsii.tests.calculator.DataRenderer;
Expand Down Expand Up @@ -50,6 +51,7 @@
import software.amazon.jsii.tests.calculator.ReturnsPrivateImplementationOfInterface;
import software.amazon.jsii.tests.calculator.StableStruct;
import software.amazon.jsii.tests.calculator.Statics;
import software.amazon.jsii.tests.calculator.StructWithJavaReservedWords;
import software.amazon.jsii.tests.calculator.Sum;
import software.amazon.jsii.tests.calculator.SyncVirtualMethods;
import software.amazon.jsii.tests.calculator.UnionProperties;
Expand Down Expand Up @@ -1018,9 +1020,29 @@ public void consts() {
@Test
public void reservedKeywordsAreSlugifiedInMethodNames() {
JavaReservedWords obj = new JavaReservedWords();
obj.import_();
obj.const_();
assertEquals("hello", obj.getWhile()); // properties do not need to be slufieid
obj.doImport();
obj.doConst();
assertEquals("hello", obj.getWhileValue()); // properties should also be 'slugified'
}

@Test
public void reservedKeywordsAreSlugifiedInStructProperties() {
StructWithJavaReservedWords struct = StructWithJavaReservedWords.builder()
.assertValue("one")
.defaultValue("two")
.build();

assertEquals("one", struct.getAssertValue());
assertEquals("two", struct.getDefaultValue());
}

@Test
public void reservedKeywordsAreSlugifiedInClassProperties() {
ClassWithJavaReservedWords obj = new ClassWithJavaReservedWords("one");

String result = obj.doImport("two");

assertEquals("onetwo", result);
}

@Test
Expand Down
83 changes: 55 additions & 28 deletions packages/jsii-pacmak/lib/targets/java.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import clone = require('clone');
import {toPascalCase} from "codemaker/lib/case-utils";
import fs = require('fs-extra');
import spec = require('jsii-spec');
import path = require('path');
Expand Down Expand Up @@ -183,6 +184,50 @@ interface JavaProp {
}

class JavaGenerator extends Generator {
// When the code-generator needs to generate code for a property or method that has the same name as a member of this list, the name will
// be automatically modified to avoid compile errors. Most of these are java language reserved keywords. In addition to those, any keywords that
// are likely to conflict with auto-generated methods or properties (eg: 'build') are also considered reserved.
private static RESERVED_KEYWORDS = [
'abstract', 'assert', 'boolean', 'break', 'build', 'byte', 'case', 'catch', 'char', 'class',
'const', 'continue', 'default', 'double', 'do', 'else', 'enum', 'extends', 'false',
'final', 'finally', 'float', 'for', 'goto', 'if', 'implements', 'import', 'instanceof',
'int', 'interface', 'long', 'native', 'new', 'null', 'package', 'private', 'protected',
'public', 'return', 'short', 'static', 'strictfp', 'super', 'switch', 'synchronized',
'this', 'throw', 'throws', 'transient', 'true', 'try', 'void', 'volatile', 'while'
];

/**
* Turns a raw javascript property name (eg: 'default') into a safe Java property name (eg: 'defaultValue').
* @param propertyName the raw JSII property Name
*/
private static safeJavaPropertyName(propertyName: string) {
if (!propertyName) {
return propertyName;
}

if (JavaGenerator.RESERVED_KEYWORDS.includes(propertyName)) {
return `${propertyName}Value`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth checking whether this is the name of an existing property on the same type... If that is so, just bail out saying you're out of options there...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Difficult to do, so in the interests of unblocking CDK builds moving ahead without this functionality for now.

} else {
return propertyName;
}
}

/**
* Turns a raw javascript method name (eg: 'import') into a safe Java method name 9eg: 'doImport').
bmaizels marked this conversation as resolved.
Show resolved Hide resolved
* @param methodName
*/
private static safeJavaMethodName(methodName: string) {
if (!methodName) {
return methodName;
}

if (JavaGenerator.RESERVED_KEYWORDS.includes(methodName)) {
return `do${toPascalCase(methodName)}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, would be nice to have a collision check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Difficult to do, so in the interests of unblocking CDK builds moving ahead without this functionality for now.

} else {
return methodName;
}
}

/** If false, @Generated will not include generator version nor timestamp */
private emitFullGeneratorInfo?: boolean;
private moduleClass: string;
Expand Down Expand Up @@ -377,7 +422,7 @@ class JavaGenerator extends Generator {
protected onInterfaceProperty(_ifc: spec.InterfaceType, prop: spec.Property) {
const getterType = this.toJavaType(prop.type);
const setterTypes = this.toJavaTypes(prop.type);
const propName = this.code.toPascalCase(prop.name);
const propName = this.code.toPascalCase(JavaGenerator.safeJavaPropertyName(prop.name));

// for unions we only generate overloads for setters, not getters.
this.addJavaDocs(prop);
Expand Down Expand Up @@ -649,7 +694,7 @@ class JavaGenerator extends Generator {
const getterType = this.toJavaType(prop.type);
const setterTypes = this.toJavaTypes(prop.type);
const propClass = this.toJavaType(prop.type, true);
const propName = this.code.toPascalCase(prop.name);
const propName = this.code.toPascalCase(JavaGenerator.safeJavaPropertyName(prop.name));
const access = this.renderAccessLevel(prop);
const statc = prop.static ? 'static ' : '';
const javaClass = this.toJavaType(cls);
Expand Down Expand Up @@ -702,7 +747,7 @@ class JavaGenerator extends Generator {
const statc = method.static ? 'static ' : '';
const access = this.renderAccessLevel(method);
const async = !!method.async;
const methodName = slugify(method.name);
const methodName = JavaGenerator.safeJavaMethodName(method.name);
const signature = `${returnType} ${methodName}(${this.renderMethodParameters(method)})`;
this.code.line();
this.addJavaDocs(method);
Expand Down Expand Up @@ -840,15 +885,16 @@ class JavaGenerator extends Generator {

function collectProps(currentIfc: spec.InterfaceType, isBaseClass = false) {
for (const property of currentIfc.properties || []) {
const propName = self.code.toPascalCase(property.name);
const safeName = JavaGenerator.safeJavaPropertyName(property.name);
const propName = self.code.toPascalCase(safeName);

const prop: JavaProp = {
docs: property.docs,
spec: property,
propName,
jsiiName: property.name,
nullable: !!property.optional,
fieldName: self.code.toCamelCase(property.name),
fieldName: self.code.toCamelCase(safeName),
fieldJavaType: self.toJavaType(property.type),
fieldJavaClass: `${self.toJavaType(property.type, true)}.class`,
javaTypes: self.toJavaTypes(property.type),
Expand Down Expand Up @@ -1236,9 +1282,10 @@ class JavaGenerator extends Generator {
}

function _renderParameter(param: spec.Parameter) {
const safeName = JavaGenerator.safeJavaPropertyName(param.name);
return isNullable(param)
? param.name
: `java.util.Objects.requireNonNull(${param.name}, "${param.name} is required")`;
? safeName
: `java.util.Objects.requireNonNull(${safeName}, "${safeName} is required")`;
}
}

Expand Down Expand Up @@ -1277,7 +1324,7 @@ class JavaGenerator extends Generator {
const params = [];
if (method.parameters) {
for (const p of method.parameters) {
params.push(`final ${this.toJavaType(p.type)}${p.variadic ? '...' : ''} ${p.name}`);
params.push(`final ${this.toJavaType(p.type)}${p.variadic ? '...' : ''} ${JavaGenerator.safeJavaPropertyName(p.name)}`);
}
}
return params.join(', ');
Expand Down Expand Up @@ -1409,26 +1456,6 @@ class JavaGenerator extends Generator {
}
}

function slugify(name?: string) {
if (!name) {
return name;
}
if (RESERVED_KEYWORDS.includes(name)) {
return `${name}_`;
} else {
return name;
}
}

const RESERVED_KEYWORDS = [
'abstract', 'assert', 'boolean', 'break', 'byte', 'case', 'catch', 'char', 'class',
'const', 'continue', 'default', 'double', 'do', 'else', 'enum', 'extends', 'false',
'final', 'finally', 'float', 'for', 'goto', 'if', 'implements', 'import', 'instanceof',
'int', 'interface', 'long', 'native', 'new', 'null', 'package', 'private', 'protected',
'public', 'return', 'short', 'static', 'strictfp', 'super', 'switch', 'synchronized',
'this', 'throw', 'throws', 'transient', 'true', 'try', 'void', 'volatile', 'while'
];

/**
* This models the POM schema for a <dependency> entity
* @see https://maven.apache.org/pom.html#Dependencies
Expand Down
Loading