Skip to content

Commit

Permalink
fix(compiler): error when type (class) defined before super() call (#…
Browse files Browse the repository at this point in the history
…6535)

This is part of the effort to address #4925.

Because inflight closures turn into class definitions, when they are passed to a `super()` call we use to get an error telling us that `super()` must be the first statement in a ctor. Now this is fixed:
```wing
class Foo {
  new(x: num) {}
}
class Bar extends Foo {
  new() {
    class Baz {} // This is now ok
    super(1);
  }
}
```
This PR also improves the diagnostics for mssing super calls or "not first statement" super calls.

note: waiting for #6531 to be merged first.

## Checklist

- [x] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted)
- [x] Description explains motivation and solution
- [x] Tests added (always)
- [ ] Docs updated (only required for features)
- [ ] Added `pr/e2e-full` label if this feature requires end-to-end testing

*By submitting this pull request, I confirm that my contribution is made under the terms of the [Wing Cloud Contribution License](https://github.com/winglang/wing/blob/main/CONTRIBUTION_LICENSE.md)*.
  • Loading branch information
yoav-steinberg authored May 24, 2024
1 parent db0720d commit 4b42f9d
Show file tree
Hide file tree
Showing 7 changed files with 240 additions and 45 deletions.
9 changes: 9 additions & 0 deletions libs/wingc/src/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,15 @@ pub enum StmtKind {
ExplicitLift(ExplicitLift),
}

impl StmtKind {
pub fn is_type_def(&self) -> bool {
matches!(
self,
StmtKind::Class(_) | StmtKind::Interface(_) | StmtKind::Struct(_) | StmtKind::Enum(_)
)
}
}

#[derive(Debug)]
pub struct ExplicitLift {
pub qualifications: Vec<LiftQualification>,
Expand Down
2 changes: 1 addition & 1 deletion libs/wingc/src/jsify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1681,7 +1681,7 @@ impl<'a> JSifier<'a> {
};

// Check if the first statement is a super constructor call, if not we need to add one
let super_called = if let Some(s) = init_statements.statements.first() {
let super_called = if let Some(s) = init_statements.statements.iter().find(|s| !s.kind.is_type_def()) {
matches!(s.kind, StmtKind::SuperConstructor { .. })
} else {
false
Expand Down
172 changes: 172 additions & 0 deletions libs/wingc/src/jsify/snapshots/allow_type_def_before_super.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
---
source: libs/wingc/src/jsify/tests.rs
---
## Code

```w
class Foo {
new(x: num) {}
}
class Bar extends Foo {
new() {
class Baz {}
super(1);
}
}
```

## inflight.Bar-1.cjs

```js
"use strict";
const $helpers = require("@winglang/sdk/lib/helpers");
module.exports = function({ $Foo }) {
class Bar extends $Foo {
constructor({ }) {
super({ });
}
}
return Bar;
}
//# sourceMappingURL=inflight.Bar-1.cjs.map
```

## inflight.Baz-1.cjs

```js
"use strict";
const $helpers = require("@winglang/sdk/lib/helpers");
module.exports = function({ }) {
class Baz {
constructor({ }) {
}
}
return Baz;
}
//# sourceMappingURL=inflight.Baz-1.cjs.map
```

## inflight.Foo-1.cjs

```js
"use strict";
const $helpers = require("@winglang/sdk/lib/helpers");
module.exports = function({ }) {
class Foo {
constructor({ }) {
}
}
return Foo;
}
//# sourceMappingURL=inflight.Foo-1.cjs.map
```

## preflight.cjs

```js
"use strict";
const $stdlib = require('@winglang/sdk');
const $platforms = ((s) => !s ? [] : s.split(';'))(process.env.WING_PLATFORMS);
const $outdir = process.env.WING_SYNTH_DIR ?? ".";
const $wing_is_test = process.env.WING_IS_TEST === "true";
const std = $stdlib.std;
const $helpers = $stdlib.helpers;
const $extern = $helpers.createExternRequire(__dirname);
class $Root extends $stdlib.std.Resource {
constructor($scope, $id) {
super($scope, $id);
class Foo extends $stdlib.std.Resource {
constructor($scope, $id, x) {
super($scope, $id);
}
static _toInflightType() {
return `
require("${$helpers.normalPath(__dirname)}/inflight.Foo-1.cjs")({
})
`;
}
_toInflight() {
return `
(await (async () => {
const FooClient = ${Foo._toInflightType()};
const client = new FooClient({
});
if (client.$inflight_init) { await client.$inflight_init(); }
return client;
})())
`;
}
get _liftMap() {
return ({
"$inflight_init": [
],
});
}
}
class Bar extends Foo {
constructor($scope, $id, ) {
class Baz extends $stdlib.std.Resource {
constructor($scope, $id, ) {
super($scope, $id);
}
static _toInflightType() {
return `
require("${$helpers.normalPath(__dirname)}/inflight.Baz-1.cjs")({
})
`;
}
_toInflight() {
return `
(await (async () => {
const BazClient = ${Baz._toInflightType()};
const client = new BazClient({
});
if (client.$inflight_init) { await client.$inflight_init(); }
return client;
})())
`;
}
get _liftMap() {
return ({
"$inflight_init": [
],
});
}
}
super($scope, $id, 1);
}
static _toInflightType() {
return `
require("${$helpers.normalPath(__dirname)}/inflight.Bar-1.cjs")({
$Foo: ${$stdlib.core.liftObject(Foo)},
})
`;
}
_toInflight() {
return `
(await (async () => {
const BarClient = ${Bar._toInflightType()};
const client = new BarClient({
});
if (client.$inflight_init) { await client.$inflight_init(); }
return client;
})())
`;
}
get _liftMap() {
return $stdlib.core.mergeLiftDeps(super._liftMap, {
"$inflight_init": [
],
});
}
}
}
}
const $PlatformManager = new $stdlib.platform.PlatformManager({platformPaths: $platforms});
const $APP = $PlatformManager.createApp({ outdir: $outdir, name: "main", rootConstruct: $Root, isTestEnvironment: $wing_is_test, entrypointDir: process.env['WING_SOURCE_DIR'], rootId: process.env['WING_ROOT_ID'] });
$APP.synth();
//# sourceMappingURL=preflight.cjs.map
```

17 changes: 17 additions & 0 deletions libs/wingc/src/jsify/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2040,3 +2040,20 @@ fn entrypoint_this() {
"#
);
}

#[test]
fn allow_type_def_before_super() {
assert_compile_ok!(
r#"
class Foo {
new(x: num) {}
}
class Bar extends Foo {
new() {
class Baz {}
super(1);
}
}
"#
);
}
15 changes: 3 additions & 12 deletions libs/wingc/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,7 @@ impl<'s> Parser<'s> {
"struct_definition" => self.build_struct_definition_statement(statement_node, phase)?,
"test_statement" => self.build_test_statement(statement_node)?,
"compiler_dbg_env" => StmtKind::CompilerDebugEnv,
"super_constructor_statement" => self.build_super_constructor_statement(statement_node, phase, idx)?,
"super_constructor_statement" => self.build_super_constructor_statement(statement_node, phase)?,
"lift_statement" => self.build_lift_statement(statement_node, phase)?,
"ERROR" => return self.with_error("Expected statement", statement_node),
other => return self.report_unimplemented_grammar(other, "statement", statement_node),
Expand Down Expand Up @@ -2616,25 +2616,16 @@ impl<'s> Parser<'s> {
}
}

fn build_super_constructor_statement(&self, statement_node: &Node, phase: Phase, idx: usize) -> Result<StmtKind, ()> {
fn build_super_constructor_statement(&self, statement_node: &Node, phase: Phase) -> Result<StmtKind, ()> {
// Calls to super constructor can only occur in specific scenario:
// 1. We are in a derived class' constructor
// 2. The statement is the first statement in the block
// We are in a derived class' constructor
let parent_block = statement_node.parent();
if let Some(p) = parent_block {
let parent_block_context = p.parent();

if let Some(context) = parent_block_context {
match context.kind() {
"initializer" | "inflight_initializer" => {
// Check that call to super constructor was first in statement block
if idx != 0 {
self.with_error(
"Call to super constructor must be first statement in constructor",
statement_node,
)?;
};

// Check that the class has a parent
let class_node = context.parent().unwrap().parent().unwrap();
let parent_class = class_node.child_by_field_name("parent");
Expand Down
48 changes: 33 additions & 15 deletions libs/wingc/src/type_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4449,21 +4449,39 @@ impl<'a> TypeChecker<'a> {
.as_function_sig()
.expect("ctor to be a function");
if let FunctionBody::Statements(ctor_body) = &ctor_def.body {
if parent_ctor_sig.min_parameters() > 0
&& !matches!(
ctor_body.statements.first(),
Some(Stmt {
kind: StmtKind::SuperConstructor { .. },
..
})
) {
self.spanned_error(
ctor_body,
format!(
"Missing super() call as first statement of {}'s constructor",
ast_class.name
),
);
// Make sure there's a `super()` call to the parent ctor
if parent_ctor_sig.min_parameters() > 0 {
// Find the `super()` call
if let Some((idx, super_call)) = ctor_body
.statements
.iter()
.enumerate()
.find(|(_, s)| matches!(s.kind, StmtKind::SuperConstructor { .. }))
{
// We have a super call, make sure it's the first statement (after any type defs)
let expected_idx = ctor_body
.statements
.iter()
.position(|s| !s.kind.is_type_def())
.expect("at least the super call stmt");
if idx != expected_idx {
self.spanned_error(
super_call,
format!(
"super() call must be the first statement of {}'s constructor",
ast_class.name
),
);
}
} else {
self.spanned_error(
ctor_body,
format!(
"Missing super() call as first statement of {}'s constructor",
ast_class.name
),
);
}
}
}
}
Expand Down
22 changes: 5 additions & 17 deletions tools/hangar/__snapshots__/invalid.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -726,13 +726,6 @@ exports[`class.test.w 1`] = `
| ^^^^^^^^


error: Call to super constructor must be first statement in constructor
--> ../../../examples/tests/invalid/class.test.w:109:5
|
109 | super(name, major);
| ^^^^^^^^^^^^^^^^^^^


error: Call to super constructor can only be done from within class constructor
--> ../../../examples/tests/invalid/class.test.w:114:5
|
Expand Down Expand Up @@ -887,16 +880,11 @@ error: Unknown symbol \\"C11\\"
| ^^^


error: Missing super() call as first statement of PaidStudent's constructor
--> ../../../examples/tests/invalid/class.test.w:107:45
|
107 | new(name: str, major: str, hrlyWage: num) {
| /---------------------------------------------^
108 | | this.hrlyWage = hrlyWage;
109 | | super(name, major);
110 | | // ^^^^^^^^^^^^^^^^^^^ Expected call to super to be first statement in constructor
111 | | }
| \\\\---^
error: super() call must be the first statement of PaidStudent's constructor
--> ../../../examples/tests/invalid/class.test.w:109:5
|
109 | super(name, major);
| ^^^^^^^^^^^^^^^^^^^


error: Cannot instantiate abstract class \\"Resource\\"
Expand Down

0 comments on commit 4b42f9d

Please sign in to comment.