Skip to content

Commit 2c39079

Browse files
mrgrainmrgrain
authored
fix: covariant method overrides break Go compilation (#4967)
Currently generated Go code is not compiled, which leads to potential codegen mistakes slipping in. Switch on compilation by default; i.e., make the opt-in environment variable an opt-out environment variable. This revealed a problem with covariant return types in Go; this concept does not exist. We'll solve that by not emitting the more specific return type. This means Go users will not benefit from the additional typing, but the feature can remain in other languages and the compilation will succeed. Go users instead get documentation telling them the real type. They can easily user interface conversions to convert to the actual type. While this is a lot awkward, it still makes it possible for Go users to benefit from this improvement in most circumstances. --- 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
1 parent ee67cc9 commit 2c39079

File tree

15 files changed

+1034
-53
lines changed

15 files changed

+1034
-53
lines changed

‎packages/@jsii/go-runtime-test/project/covariantoverrides_test.go‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
package tests
2+
3+
import (
4+
"testing"
5+
6+
"github.com/aws/jsii/jsii-calc/go/jsiicalc/v3/covariantoverrides/classoverrides"
7+
)
8+
9+
func TestBase(t *testing.T) {
10+
base := classoverrides.NewBase()
11+
if *base.Something().Name() != "Superclass" {
12+
t.Errorf("Expected Superclass, got %s", *base.Something().Name())
13+
}
14+
if *base.CreateSomething().SayHello() != "Hello Superclass" {
15+
t.Errorf("Expected 'Hello Superclass', got %s", *base.CreateSomething().SayHello())
16+
}
17+
}
18+
19+
func TestMiddle(t *testing.T) {
20+
middle := classoverrides.NewMiddle()
21+
if *middle.Something().Name() != "Superclass" {
22+
t.Errorf("Expected Superclass, got %s", *middle.Something().Name())
23+
}
24+
if *middle.CreateSomething().SayHello() != "Hello Superclass" {
25+
t.Errorf("Expected 'Hello Superclass', got %s", *middle.CreateSomething().SayHello())
26+
}
27+
}
28+
29+
func TestDerived(t *testing.T) {
30+
derived := classoverrides.NewDerived()
31+
if *derived.Something().Name() != "SubSubclass" {
32+
t.Errorf("Expected SubSubclass, got %s", *derived.Something().Name())
33+
}
34+
created := derived.CreateSomething()
35+
if *created.Name() != "SubSubclass" {
36+
t.Errorf("Expected SubSubclass, got %s", *created.Name())
37+
}
38+
if *created.SayHello() != "Hello SubSubclass" {
39+
t.Errorf("Expected 'Hello SubSubclass', got %s", *created.SayHello())
40+
}
41+
}
42+
43+
func TestPolymorphism(t *testing.T) {
44+
derived := classoverrides.NewDerived()
45+
var base classoverrides.IBase = derived
46+
47+
if *base.Something().Name() != "SubSubclass" {
48+
t.Errorf("Expected SubSubclass, got %s", *base.Something().Name())
49+
}
50+
}
51+
52+
func consumeSubSubclass(s classoverrides.SubSubclass) string {
53+
return *s.Name()
54+
}
55+
56+
func TestTypeCasting(t *testing.T) {
57+
derived := classoverrides.NewDerived()
58+
superclassResult := derived.Something()
59+
name := consumeSubSubclass(superclassResult.(classoverrides.SubSubclass))
60+
if name != "SubSubclass" {
61+
t.Errorf("Expected SubSubclass, got %s", name)
62+
}
63+
}
64+

‎packages/jsii-calc/lib/covariant-overrides/class-overrides.ts‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,33 @@
66
*/
77

88
/** Base class in the inheritance hierarchy */
9-
export class Superclass {}
9+
export class Superclass {
10+
public name = 'Superclass';
11+
public sayHello(): string {
12+
return 'Hello Superclass';
13+
}
14+
}
1015

1116
/** Derived class that extends Superclass */
12-
export class Subclass extends Superclass {}
17+
export class Subclass extends Superclass {
18+
public name = 'Subclass';
19+
public sayHello(): string {
20+
return 'Hello Subclass';
21+
}
22+
}
1323

1424
/** Further derived class that extends Subclass */
15-
export class SubSubclass extends Subclass {}
25+
export class SubSubclass extends Subclass {
26+
/**
27+
* Ensures this class is different than its parent.
28+
*/
29+
public unique = 'yes';
30+
31+
public name = 'SubSubclass';
32+
public sayHello(): string {
33+
return 'Hello SubSubclass';
34+
}
35+
}
1636

1737
export interface IBase {
1838
readonly something: Superclass;

‎packages/jsii-calc/test/assembly.jsii‎

Lines changed: 129 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -16749,7 +16749,7 @@
1674916749
"kind": "class",
1675016750
"locationInModule": {
1675116751
"filename": "lib/covariant-overrides/class-overrides.ts",
16752-
"line": 22
16752+
"line": 42
1675316753
},
1675416754
"methods": [
1675516755
{
@@ -16758,7 +16758,7 @@
1675816758
},
1675916759
"locationInModule": {
1676016760
"filename": "lib/covariant-overrides/class-overrides.ts",
16761-
"line": 26
16761+
"line": 46
1676216762
},
1676316763
"name": "createSomething",
1676416764
"returns": {
@@ -16778,7 +16778,7 @@
1677816778
"immutable": true,
1677916779
"locationInModule": {
1678016780
"filename": "lib/covariant-overrides/class-overrides.ts",
16781-
"line": 24
16781+
"line": 44
1678216782
},
1678316783
"name": "list",
1678416784
"type": {
@@ -16797,7 +16797,7 @@
1679716797
"immutable": true,
1679816798
"locationInModule": {
1679916799
"filename": "lib/covariant-overrides/class-overrides.ts",
16800-
"line": 23
16800+
"line": 43
1680116801
},
1680216802
"name": "something",
1680316803
"overrides": "jsii-calc.covariantOverrides.classOverrides.IBase",
@@ -16825,7 +16825,7 @@
1682516825
"kind": "class",
1682616826
"locationInModule": {
1682716827
"filename": "lib/covariant-overrides/class-overrides.ts",
16828-
"line": 42
16828+
"line": 62
1682916829
},
1683016830
"methods": [
1683116831
{
@@ -16834,7 +16834,7 @@
1683416834
},
1683516835
"locationInModule": {
1683616836
"filename": "lib/covariant-overrides/class-overrides.ts",
16837-
"line": 48
16837+
"line": 68
1683816838
},
1683916839
"name": "createSomething",
1684016840
"overrides": "jsii-calc.covariantOverrides.classOverrides.Base",
@@ -16855,7 +16855,7 @@
1685516855
"immutable": true,
1685616856
"locationInModule": {
1685716857
"filename": "lib/covariant-overrides/class-overrides.ts",
16858-
"line": 45
16858+
"line": 65
1685916859
},
1686016860
"name": "list",
1686116861
"overrides": "jsii-calc.covariantOverrides.classOverrides.Base",
@@ -16875,7 +16875,7 @@
1687516875
"immutable": true,
1687616876
"locationInModule": {
1687716877
"filename": "lib/covariant-overrides/class-overrides.ts",
16878-
"line": 44
16878+
"line": 64
1687916879
},
1688016880
"name": "something",
1688116881
"overrides": "jsii-calc.covariantOverrides.classOverrides.Base",
@@ -16895,7 +16895,7 @@
1689516895
"kind": "interface",
1689616896
"locationInModule": {
1689716897
"filename": "lib/covariant-overrides/class-overrides.ts",
16898-
"line": 17
16898+
"line": 37
1689916899
},
1690016900
"name": "IBase",
1690116901
"namespace": "covariantOverrides.classOverrides",
@@ -16908,7 +16908,7 @@
1690816908
"immutable": true,
1690916909
"locationInModule": {
1691016910
"filename": "lib/covariant-overrides/class-overrides.ts",
16911-
"line": 18
16911+
"line": 38
1691216912
},
1691316913
"name": "something",
1691416914
"type": {
@@ -16934,7 +16934,7 @@
1693416934
"kind": "class",
1693516935
"locationInModule": {
1693616936
"filename": "lib/covariant-overrides/class-overrides.ts",
16937-
"line": 32
16937+
"line": 52
1693816938
},
1693916939
"name": "Middle",
1694016940
"namespace": "covariantOverrides.classOverrides",
@@ -16945,7 +16945,7 @@
1694516945
},
1694616946
"locationInModule": {
1694716947
"filename": "lib/covariant-overrides/class-overrides.ts",
16948-
"line": 33
16948+
"line": 53
1694916949
},
1695016950
"name": "addUnrelatedMember",
1695116951
"type": {
@@ -16971,10 +16971,58 @@
1697116971
"kind": "class",
1697216972
"locationInModule": {
1697316973
"filename": "lib/covariant-overrides/class-overrides.ts",
16974-
"line": 15
16974+
"line": 25
1697516975
},
16976+
"methods": [
16977+
{
16978+
"docs": {
16979+
"stability": "stable"
16980+
},
16981+
"locationInModule": {
16982+
"filename": "lib/covariant-overrides/class-overrides.ts",
16983+
"line": 32
16984+
},
16985+
"name": "sayHello",
16986+
"overrides": "jsii-calc.covariantOverrides.classOverrides.Subclass",
16987+
"returns": {
16988+
"type": {
16989+
"primitive": "string"
16990+
}
16991+
}
16992+
}
16993+
],
1697616994
"name": "SubSubclass",
1697716995
"namespace": "covariantOverrides.classOverrides",
16996+
"properties": [
16997+
{
16998+
"docs": {
16999+
"stability": "stable"
17000+
},
17001+
"locationInModule": {
17002+
"filename": "lib/covariant-overrides/class-overrides.ts",
17003+
"line": 31
17004+
},
17005+
"name": "name",
17006+
"overrides": "jsii-calc.covariantOverrides.classOverrides.Subclass",
17007+
"type": {
17008+
"primitive": "string"
17009+
}
17010+
},
17011+
{
17012+
"docs": {
17013+
"stability": "stable",
17014+
"summary": "Ensures this class is different than its parent."
17015+
},
17016+
"locationInModule": {
17017+
"filename": "lib/covariant-overrides/class-overrides.ts",
17018+
"line": 29
17019+
},
17020+
"name": "unique",
17021+
"type": {
17022+
"primitive": "string"
17023+
}
17024+
}
17025+
],
1697817026
"symbolId": "lib/covariant-overrides/class-overrides:SubSubclass"
1697917027
},
1698017028
"jsii-calc.covariantOverrides.classOverrides.Subclass": {
@@ -16993,10 +17041,44 @@
1699317041
"kind": "class",
1699417042
"locationInModule": {
1699517043
"filename": "lib/covariant-overrides/class-overrides.ts",
16996-
"line": 12
17044+
"line": 17
1699717045
},
17046+
"methods": [
17047+
{
17048+
"docs": {
17049+
"stability": "stable"
17050+
},
17051+
"locationInModule": {
17052+
"filename": "lib/covariant-overrides/class-overrides.ts",
17053+
"line": 19
17054+
},
17055+
"name": "sayHello",
17056+
"overrides": "jsii-calc.covariantOverrides.classOverrides.Superclass",
17057+
"returns": {
17058+
"type": {
17059+
"primitive": "string"
17060+
}
17061+
}
17062+
}
17063+
],
1699817064
"name": "Subclass",
1699917065
"namespace": "covariantOverrides.classOverrides",
17066+
"properties": [
17067+
{
17068+
"docs": {
17069+
"stability": "stable"
17070+
},
17071+
"locationInModule": {
17072+
"filename": "lib/covariant-overrides/class-overrides.ts",
17073+
"line": 18
17074+
},
17075+
"name": "name",
17076+
"overrides": "jsii-calc.covariantOverrides.classOverrides.Superclass",
17077+
"type": {
17078+
"primitive": "string"
17079+
}
17080+
}
17081+
],
1700017082
"symbolId": "lib/covariant-overrides/class-overrides:Subclass"
1700117083
},
1700217084
"jsii-calc.covariantOverrides.classOverrides.Superclass": {
@@ -17016,8 +17098,40 @@
1701617098
"filename": "lib/covariant-overrides/class-overrides.ts",
1701717099
"line": 9
1701817100
},
17101+
"methods": [
17102+
{
17103+
"docs": {
17104+
"stability": "stable"
17105+
},
17106+
"locationInModule": {
17107+
"filename": "lib/covariant-overrides/class-overrides.ts",
17108+
"line": 11
17109+
},
17110+
"name": "sayHello",
17111+
"returns": {
17112+
"type": {
17113+
"primitive": "string"
17114+
}
17115+
}
17116+
}
17117+
],
1701917118
"name": "Superclass",
1702017119
"namespace": "covariantOverrides.classOverrides",
17120+
"properties": [
17121+
{
17122+
"docs": {
17123+
"stability": "stable"
17124+
},
17125+
"locationInModule": {
17126+
"filename": "lib/covariant-overrides/class-overrides.ts",
17127+
"line": 10
17128+
},
17129+
"name": "name",
17130+
"type": {
17131+
"primitive": "string"
17132+
}
17133+
}
17134+
],
1702117135
"symbolId": "lib/covariant-overrides/class-overrides:Superclass"
1702217136
},
1702317137
"jsii-calc.homonymousForwardReferences.bar.Consumer": {
@@ -19672,5 +19786,5 @@
1967219786
"intersection-types"
1967319787
],
1967419788
"version": "3.20.120",
19675-
"fingerprint": "bgdsE4i6uPRf3d8FnIARS1FO4PilyDwzanABdH+SqIg="
19789+
"fingerprint": "6Vy4e6a5fe6HlMnZVVfUxQ6kP2NgQEfEjwgiDHv/Ssw="
1967619790
}

‎packages/jsii-pacmak/lib/targets/go.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,13 @@ export class Golang extends Target {
5656
return Promise.reject(e);
5757
}
5858

59-
if (process.env.JSII_BUILD_GO) {
60-
// This step is taken to ensure that the generated code is compilable
59+
// This step is taken to ensure that the generated code is compilable
60+
// Do it by default, because we didn't use to and that was masking a code generation
61+
// problem.
62+
const doBuild = !['false', '0', 'no'].includes(
63+
process.env.JSII_BUILD_GO ?? '1',
64+
);
65+
if (doBuild) {
6166
await go('build', ['-modfile', localGoMod.path, './...'], {
6267
cwd: pkgDir,
6368
});

0 commit comments

Comments
 (0)