Skip to content

Commit

Permalink
fix(java): type generation code-gen issues (#3735)
Browse files Browse the repository at this point in the history 
Some collection validations were emitted twice, due to a bad merge
conflict resolution.

Additionally, in certain cases where overrides are being emitted for
type unions, the type checking code made unnecessary assertions, some of
which ending up being prohibited by the compiler.



---

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
  • Loading branch information
@RomainMuller
RomainMuller authored Aug 30, 2022
1 parent 4c2dcd5 commit cf04f79
Show file tree
Hide file tree
Showing 3 changed files with 217 additions and 259 deletions.
103 changes: 61 additions & 42 deletions .github/workflows/main.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ jobs:
- name: Set up Java 8
uses: actions/setup-java@v3
with:
java-version: '8'
distribution: 'zulu'
java-version: '8'
- name: Set up Node 14
uses: actions/setup-node@v3
with:
Expand All @@ -50,30 +50,19 @@ jobs:
uses: actions/setup-python@v4
with:
python-version: '3.7'
cache: pip
- name: Install python3-venv
run: sudo apt install -y python3-venv
- name: Locate Caches
id: cache-locations
run: |-
echo "::group::Upgrade pip"
# Need to have PIP >= 20.1 for "pip cache dir" to work
python3 -m pip install --upgrade pip
echo "::endgroup"
echo "::set-output name=pip-cache::$(python3 -m pip cache dir)"
- name: Cache
uses: actions/cache@v3
with:
path: |-
${{ steps.cache-locations.outputs.pip-cache }}
~/.m2/repository
!~/.m2/repository/software/amazon/jsii/
~/.nuget/packages
!~/.nuget/packages/amazon.jsii.*
key: ${{ runner.os }}-node@14-python@3.7-${{ hashFiles('**/yarn.lock', '**/Directory.Build.targets') }}
key: ${{ runner.os }}-${{ hashFiles('**/Directory.Build.targets') }}
restore-keys: |-
${{ runner.os }}-node@14-python@3.7-
${{ runner.os }}-node@14-
${{ runner.os }}-
# Prepare dependencies and build
- name: Install Dependencies
Expand Down Expand Up @@ -133,8 +122,8 @@ jobs:
- name: Set up Java 8
uses: actions/setup-java@v3
with:
java-version: '8'
distribution: 'zulu'
java-version: '8'
- name: Set up Node 14
uses: actions/setup-node@v3
with:
Expand All @@ -144,30 +133,19 @@ jobs:
uses: actions/setup-python@v4
with:
python-version: '3.7'
cache: pip
- name: Install python3-venv
run: sudo apt install -y python3-venv
- name: Locate Caches
id: cache-locations
run: |-
echo "::group::Upgrade pip"
# Need to have PIP >= 20.1 for "pip cache dir" to work
python3 -m pip install --upgrade pip
echo "::endgroup"
echo "::set-output name=pip-cache::$(python3 -m pip cache dir)"
- name: Cache
uses: actions/cache@v3
with:
path: |-
${{ steps.cache-locations.outputs.pip-cache }}
~/.m2/repository
!~/.m2/repository/software/amazon/jsii/
~/.nuget/packages
!~/.nuget/packages/amazon.jsii.*
key: ${{ runner.os }}-node@14-python@3.7-${{ hashFiles('**/yarn.lock', '**/Directory.Build.targets') }}
key: ${{ runner.os }}-${{ hashFiles('**/Directory.Build.targets') }}
restore-keys: |-
${{ runner.os }}-node@14-python@3.7-
${{ runner.os }}-node@14-
${{ runner.os }}-
# Prepare dependencies and build
- name: Install Dependencies
Expand Down Expand Up @@ -319,8 +297,8 @@ jobs:
- name: Set up Java ${{ matrix.java }}
uses: actions/setup-java@v3
with:
java-version: ${{ matrix.java }}
distribution: 'zulu'
java-version: ${{ matrix.java }}
- name: Set up Node ${{ matrix.node }}
uses: actions/setup-node@v3
with:
Expand All @@ -330,32 +308,21 @@ jobs:
uses: actions/setup-python@v4
with:
python-version: ${{ matrix.python }}
cache: pip
- name: 'Linux: Install python3-venv'
if: runner.os == 'Linux'
run: sudo apt install -y python3-venv
- name: Locate Caches
id: cache-locations
run: |-
echo "::group::Upgrade pip"
# Need to have PIP >= 20.1 for "pip cache dir" to work
python3 -m pip install --upgrade pip
echo "::endgroup"
echo "::set-output name=pip-cache::$(python3 -m pip cache dir)"
- name: Cache
uses: actions/cache@v3
with:
path: |-
${{ steps.cache-locations.outputs.pip-cache }}
~/.m2/repository
!~/.m2/repository/software/amazon/jsii/
~/.nuget/packages
!~/.nuget/packages/amazon.jsii.*
# Not including .NET / Java in the cache keys, those artifacts are SDK-version-independent
key: ${{ runner.os }}-node@${{ matrix.node }}-python@${{ matrix.python }}-${{ hashFiles('**/yarn.lock', '**/Directory.Build.targets') }}
key: ${{ runner.os }}-${{ hashFiles('**/Directory.Build.targets') }}
restore-keys: |-
${{ runner.os }}-node@${{ matrix.node }}-python@${{ matrix.python }}-
${{ runner.os }}-node@${{ matrix.node }}-
${{ runner.os }}-
# Run the tests
- name: Install Dependencies
Expand Down Expand Up @@ -426,3 +393,55 @@ jobs:
output-file-path: ${{ runner.temp }}/bench-output.json
github-token: ${{ secrets.PROJEN_GITHUB_TOKEN }}
auto-push: true

pacmak-integration-test:
name: Integration test (jsii-pacmak)
runs-on: ubuntu-latest
needs: create-release-package
steps:
# Check out the code
- name: Download Artifact
uses: actions/download-artifact@v3
with:
name: release-package
path: ${{ runner.temp }}/release-package
# Set up all of our standard runtimes
- name: Set up .NET 6
uses: actions/setup-dotnet@v2
with:
dotnet-version: '6.0.x'
- name: Set up Go 1.18
uses: actions/setup-go@v3
with:
go-version: '1.18'
- name: Set up Java 8
uses: actions/setup-java@v3
with:
distribution: 'zulu'
java-version: '8'
- name: Set up Node 14
uses: actions/setup-node@v3
with:
node-version: '14'
- name: Set up Python 3.7
uses: actions/setup-python@v4
with:
python-version: '3.7'
- name: Install python3-venv
run: sudo apt install -y python3-venv
# Show time!
- name: Prepare Work Tree
run: |-
npm install --no-save aws-cdk-lib@2 constructs@10 \
${{ runner.temp }}/release-package/js/*.tgz \
${{ runner.temp }}/release-package/private/*.tgz
- name: Run jsii-pacmak on aws-cdk-lib
run: |-
./node_modules/.bin/jsii-pacmak ./node_modules/aws-cdk-lib
# Upload artifact (we'll tar it up to save time)
- name: 'Upload Artifact: integtest_aws-cdk-lib'
uses: actions/upload-artifact@v3
with:
name: integtest_aws-cdk-lib
path: ./node_modules/aws-cdk-lib/dist/

172 changes: 118 additions & 54 deletions packages/jsii-pacmak/lib/targets/java.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as spec from '@jsii/spec';
import * as assert from 'assert';
import * as clone from 'clone';
import { toSnakeCase } from 'codemaker/lib/case-utils';
import { createHash } from 'crypto';
Expand Down Expand Up @@ -1472,9 +1473,20 @@ class JavaGenerator extends Generator {
// so we should not emit these checks for primitive-only unions.
// Also, Java does not allow us to perform these checks if the types
// have no overlap (eg if a String instanceof Number).
if (type.includes('java.lang.Object')) {
if (
type.includes('java.lang.Object') &&
(!spec.isPrimitiveTypeReference(prop.type) ||
prop.type.primitive === spec.PrimitiveType.Any)
) {
this.emitUnionParameterValdation([
{ name: 'value', type: prop.type },
{
name: 'value',
type: this.filterType(
prop.type,
{ covariant: prop.static, optional: prop.optional },
type,
),
},
]);
}
if (prop.static) {
Expand All @@ -1493,6 +1505,40 @@ class JavaGenerator extends Generator {
}
}

/**
* Filters types from a union to select only those that correspond to the
* specified javaType.
*
* @param ref the type to be filtered.
* @param javaType the java type that is expected.
* @param covariant whether collections should use the covariant form.
* @param optional whether the type at an optional location or not
*
* @returns a type reference that matches the provided javaType.
*/
private filterType(
ref: spec.TypeReference,
{ covariant, optional }: { covariant?: boolean; optional?: boolean },
javaType: string,
): spec.TypeReference {
if (!spec.isUnionTypeReference(ref)) {
// No filterning needed -- this isn't a type union!
return ref;
}
const types = ref.union.types.filter(
(t) =>
this.toDecoratedJavaType({ optional, type: t }, { covariant }) ===
javaType,
);
assert(
types.length > 0,
`No type found in ${spec.describeTypeReference(
ref,
)} has Java type ${javaType}`,
);
return { union: { types } };
}

private emitMethod(
cls: spec.Type,
method: spec.Method,
Expand Down Expand Up @@ -1715,69 +1761,87 @@ class JavaGenerator extends Generator {
type: spec.UnionTypeReference,
parameterName: string,
) {
this.code.indent('if (');
let emitAnd = false;
const nestedCollectionUnionTypes = [];
const nestedCollectionUnionTypes = new Map<string, spec.TypeReference>();
const typeRefs = type.union.types;
if (typeRefs.length > 1) {
this.code.indent('if (');
}
const checked = new Set<string>();
for (const typeRef of typeRefs) {
const prefix = emitAnd ? '&&' : '';
const javaType = this.toJavaTypeNoGenerics(typeRef);
if (javaType !== this.toJavaType(typeRef)) {
nestedCollectionUnionTypes.push(typeRef);
const javaRawType = this.toJavaTypeNoGenerics(typeRef);
if (checked.has(javaRawType)) {
continue;
} else {
checked.add(javaRawType);
}
const javaType = this.toJavaType(typeRef);
if (javaRawType !== javaType) {
nestedCollectionUnionTypes.set(javaType, typeRef);
}
const test = `${value} instanceof ${javaRawType}`;
if (typeRefs.length > 1) {
this.code.line(`${prefix} !(${test})`);
}
const test = `${value} instanceof ${javaType}`;
this.code.line(`${prefix} !(${test})`);
emitAnd = true;
}
// Only anonymous objects at runtime can be `JsiiObject`s.
this.code.line(
`&& !(${value}.getClass().equals(software.amazon.jsii.JsiiObject.class))`,
);

this.code.unindent(')');
this.code.openBlock('');

const placeholders = typeRefs
.map((typeRef) => {
return `${this.toJavaType(typeRef)}`;
})
.join(', ');

this.code.indent(`throw new IllegalArgumentException(`);
this.code.indent(`new java.lang.StringBuilder("Expected ")`);
this.code.line(descr);
this.code.line(`.append(" to be one of: ${placeholders}; received ")`);
this.code.line(`.append(${value}.getClass()).toString());`);
this.code.unindent(false);
this.code.unindent(false);
this.code.closeBlock();

for (const typeRef of nestedCollectionUnionTypes) {
this.code.openBlock(
`if (${value} instanceof ${this.toJavaTypeNoGenerics(typeRef)})`,
);
const varName = `__cast_${createHash('sha256')
.update(value)
.digest('hex')
.slice(0, 6)}`;
const javaTypeFull = this.toJavaType(typeRef);
this.code.line(`@SuppressWarnings("unchecked")`);
if (
typeRefs.length > 1 &&
typeRefs.some(
(t) =>
spec.isNamedTypeReference(t) &&
spec.isInterfaceType(this.findType(t.fqn)),
)
) {
// Only anonymous objects at runtime can be `JsiiObject`s.
this.code.line(
`final ${javaTypeFull} ${varName} = (${javaTypeFull})${value};`,
);
validate.call(this, varName, descr, typeRef, parameterName);
validate.call(
this,
// we must cast Collections to access their methods
// if they are nested in a union type,
// since that union will be rendered as an Object.
`((${this.toJavaType(typeRef)})(${varName}))`,
descr,
typeRef,
parameterName,
`&& !(${value}.getClass().equals(software.amazon.jsii.JsiiObject.class))`,
);
}

if (typeRefs.length > 1) {
this.code.unindent(false);
this.code.openBlock(')');

const placeholders = typeRefs
.map((typeRef) => {
return `${this.toJavaType(typeRef)}`;
})
.join(', ');

this.code.indent(`throw new IllegalArgumentException(`);
this.code.indent(`new java.lang.StringBuilder("Expected ")`);
this.code.line(descr);
this.code.line(`.append(" to be one of: ${placeholders}; received ")`);
this.code.line(`.append(${value}.getClass()).toString());`);
this.code.unindent(false);
this.code.unindent(false);
this.code.closeBlock();
}

for (const [javaType, typeRef] of nestedCollectionUnionTypes) {
const varName =
typeRefs.length > 1
? `__cast_${createHash('sha256')
.update(value)
.digest('hex')
.slice(0, 6)}`
: value;
if (typeRefs.length > 1) {
this.code.openBlock(
`if (${value} instanceof ${this.toJavaTypeNoGenerics(typeRef)})`,
);
this.code.line(`@SuppressWarnings("unchecked")`);
this.code.line(
`final ${javaType} ${varName} = (${javaType})${value};`,
);
}
validate.call(this, varName, descr, typeRef, parameterName);
if (typeRefs.length > 1) {
this.code.closeBlock();
}
}
}
}

Expand Down
Loading

0 comments on commit cf04f79

Please sign in to comment.