Skip to content

Commit

Permalink
fix(go): unused imports emitted for type unions (#3664)
Browse files Browse the repository at this point in the history
When imported types are solely referenced by type unions, a go import
is emitted, but is never used (type unions end up represented as opaque
`interface{}` type). This causes compilation failures.

Added a test case for this scenario in particular, and adjusted go code
generation to not emit dependency imports for type unions.

These imports may be re-introduced soon, as we are working to add
dynamic type checking around type unions in go (at which point those
imports would no longer be unused).

Fixes #3399
  • Loading branch information
RomainMuller committed Jul 21, 2022
1 parent 3abe20f commit 68a80d9
Show file tree
Hide file tree
Showing 5 changed files with 148 additions and 3 deletions.
1 change: 1 addition & 0 deletions packages/jsii-pacmak/jest.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@ import { overriddenConfig } from '../../jest.config.mjs';

export default overriddenConfig({
coveragePathIgnorePatterns: ['/node_modules/', '<rootDir>/test'],
watchPathIgnorePatterns: ['.*\\.tsx?$'],
});
10 changes: 7 additions & 3 deletions packages/jsii-pacmak/lib/targets/go/types/go-type-reference.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,13 @@ export class GoTypeRef {
break;

case 'union':
for (const t of this.typeMap.value) {
ret.push(...t.dependencies);
}
// Unions ultimately result in `interface{}` being rendered, so no import is needed. We
// hence ignore them entirely here for now. In the future, we may want to inject specific
// runtime type checks around use of unions, which may result in imports being useful.

// for (const t of this.typeMap.value) {
// ret.push(...t.dependencies);
// }
break;

case 'void':
Expand Down
42 changes: 42 additions & 0 deletions packages/jsii-pacmak/test/targets/fixtures/base.jsii.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
{
"author": {
"email": "aws-jsii@amazon.com",
"name": "Amazon Web Services, Inc.",
"organization": true,
"roles": [
"owner"
]
},
"description": "A dummy test package",
"fingerprint": "PHONY",
"homepage": "https://aws.github.io/jsii",
"jsiiVersion": "0.0.0-dev",
"license": "Apache-2.0",
"name": "base",
"repository": {
"directory": "packages/jsii-pacmak/test/targets/fixtures",
"type": "git",
"url": "https://github.com/aws/jsii"
},
"schema": "jsii/0.10.0",
"targets": {
"go": {
"moduleName": "github.com/aws/jsii/packages/jsii-pacmak/test/targets/fixtures"
}
},
"types": {
"BaseA": {
"assembly": "base",
"fqn": "base.BaseA",
"kind": "interface",
"name": "BaseA"
},
"BaseB": {
"assembly": "base",
"fqn": "base.BaseB",
"kind": "interface",
"name": "BaseB"
}
},
"version": "1.2.3"
}
58 changes: 58 additions & 0 deletions packages/jsii-pacmak/test/targets/fixtures/dependent.jsii.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
{
"author": {
"email": "aws-jsii@amazon.com",
"name": "Amazon Web Services, Inc.",
"organization": true,
"roles": [
"owner"
]
},
"dependencies": {
"base": "1.2.3"
},
"description": "A dummy test package",
"fingerprint": "PHONY",
"homepage": "https://aws.github.io/jsii",
"jsiiVersion": "0.0.0-dev",
"license": "Apache-2.0",
"name": "dependent",
"repository": {
"directory": "packages/jsii-pacmak/test/targets/fixtures",
"type": "git",
"url": "https://github.com/aws/jsii"
},
"schema": "jsii/0.10.0",
"targets": {
"go": {
"moduleName": "github.com/aws/jsii/packages/jsii-pacmak/test/targets/fixtures"
}
},
"types": {
"Dependent": {
"assembly": "dependent",
"fqn": "dependent.Dependent",
"kind": "class",
"name": "dependent",
"methods": [
{
"name": "getBaseUnion",
"returns": {
"type": {
"union": {
"types": [
{
"fqn": "base.BaseA"
},
{
"fqn": "base.BaseB"
}
]
}
}
}
}
]
}
},
"version": "4.5.6"
}
40 changes: 40 additions & 0 deletions packages/jsii-pacmak/test/targets/go.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { promises as fs } from 'fs';
import { TypeSystem } from 'jsii-reflect';
import { Rosetta } from 'jsii-rosetta';
import { tmpdir } from 'os';
import { join } from 'path';

import { Golang } from '../../lib/targets/go';

test('does not generate imports for unused types', async () => {
const outDir = await fs.mkdtemp(join(tmpdir(), 'pacmak-golang-'));
try {
const tarball = join(outDir, 'mock-tarball.tgz');
await fs.writeFile(tarball, 'Mock Tarball');

const typeSystem = new TypeSystem();
await typeSystem.load(require.resolve('./fixtures/base.jsii.json'));
const assembly = await typeSystem.load(
require.resolve('./fixtures/dependent.jsii.json'),
);

const rosetta = new Rosetta();
const subject = new Golang({
arguments: {},
assembly,
packageDir: '',
rosetta,
targetName: 'golang',
});

await subject.generateCode(outDir, tarball);

await expect(
fs.readFile(join(outDir, assembly.name, `${assembly.name}.go`), 'utf-8'),
).resolves.not.toContain(
'github.com/aws/jsii/packages/jsii-pacmak/test/targets/fixtures/base',
);
} finally {
await fs.rm(outDir, { recursive: true });
}
});

0 comments on commit 68a80d9

Please sign in to comment.