Skip to content

Commit

Permalink
fix(compiler)!: non-static extern methods are not supported (#4027)
Browse files Browse the repository at this point in the history
It's not currently possible for extern method implementations to access class members (like fields, or other instance methods), yet it has still been possible to declare them both ways. This PR introduces an error message if you attempt to add a non-static extern method. In the future, this restriction can be lifted if the capability is supported through a new calling convention with extern JavaScript files or something like that.

## 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
Chriscbr authored Aug 31, 2023
1 parent 8cad1fa commit ca6d5be
Show file tree
Hide file tree
Showing 9 changed files with 44 additions and 15 deletions.
2 changes: 2 additions & 0 deletions docs/docs/03-language-reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -1762,6 +1762,8 @@ exports.makeId = function () {
Given a method of name X, the compiler will map the method to the JavaScript export with the
matching name (without any case conversion).
Extern methods do not support access to class's members through `this`, so they must be declared `static`.
### 5.2.1 TypeScript
It is possible to use TypeScript to write helpers, but at the moment this is not
Expand Down
3 changes: 1 addition & 2 deletions examples/tests/invalid/extern.w
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,4 @@ class Foo {
extern "./sad.js" static getNum(): num;
//^ "./sad.js" not found
extern "not-installed" static tooBad(): bool;
//^ "not-installed" not found
}
}
4 changes: 4 additions & 0 deletions examples/tests/invalid/extern_static.w
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
class Foo {
extern "../external_js.js" inflight getGreeting(name: str): str;
//^ Error: extern methods must be declared "static"
}
4 changes: 2 additions & 2 deletions examples/tests/valid/dynamo.w
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,11 @@ class DynamoTable {
}
}

extern "./dynamo.js" inflight _putItem(tableName: str, item: Json): void;
extern "./dynamo.js" static inflight _putItem(tableName: str, item: Json): void;

inflight putItem(item: Map<Attribute>) {
let json = this._itemToJson(item);
this._putItem(this.tableName, json);
DynamoTable._putItem(this.tableName, json);
}

inflight _itemToJson(item: Map<Attribute>): Json {
Expand Down
8 changes: 4 additions & 4 deletions examples/tests/valid/dynamo_awscdk.w
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,16 @@ class DynamoTable {
}
}

extern "./dynamo.js" inflight _putItem(tableName: str, item: Json): void;
extern "./dynamo.js" static inflight _putItem(tableName: str, item: Json): void;
inflight putItem(item: Map<Attribute>) {
let json = this._itemToJson(item);
this._putItem(this.tableName, json);
DynamoTable._putItem(this.tableName, json);
}

extern "./dynamo.js" inflight _getItem(tableName: str, key: Json): Json;
extern "./dynamo.js" static inflight _getItem(tableName: str, key: Json): Json;
inflight getItem(key: Map<Attribute>): Json {
let json = this._itemToJson(key);
return this._getItem(this.tableName, json);
return DynamoTable._getItem(this.tableName, json);
}

inflight _itemToJson(item: Map<Attribute>): Json {
Expand Down
4 changes: 2 additions & 2 deletions examples/tests/valid/extern_implementation.w
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ class Foo {
extern "./external_js.js" static inflight regexInflight(pattern: str, text: str): bool;
extern "./external_js.js" static inflight getUuid(): str;
extern "./external_js.js" static inflight getData(): str;
extern "./external_js.js" inflight print(msg: str);
extern "./external_js.js" static inflight print(msg: str);
extern "uuid" static v4(): str;

inflight call() {
Expand All @@ -27,5 +27,5 @@ test "call" {
}

test "console" {
f.print("hey there");
Foo.print("hey there");
}
9 changes: 9 additions & 0 deletions libs/wingc/src/type_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3941,6 +3941,15 @@ impl<'a> TypeChecker<'a> {
self.types.set_scope_env(scope, method_env);
self.inner_scopes.push(scope);
}

if let FunctionBody::External(_) = &method_def.body {
if !method_def.is_static {
self.spanned_error(
method_name,
"Extern methods must be declared \"static\" (they cannot access instance members)",
);
}
}
}

fn add_method_to_class_env(
Expand Down
15 changes: 15 additions & 0 deletions tools/hangar/__snapshots__/invalid.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -752,6 +752,21 @@ error: Failed to resolve extern \\"not-installed\\": Not Found
Tests 1 failed (1)
Test Files 1 failed (1)
Duration <DURATION>"
`;
exports[`extern_static.w 1`] = `
"error: Extern methods must be declared \\"static\\" (they cannot access instance members)
--> ../../../examples/tests/invalid/extern_static.w:2:39
|
2 | extern \\"../external_js.js\\" inflight getGreeting(name: str): str;
| ^^^^^^^^^^^ Extern methods must be declared \\"static\\" (they cannot access instance members)
Tests 1 failed (1)
Test Files 1 failed (1)
Duration <DURATION>"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,15 @@ module.exports = function({ $f }) {

## inflight.$Closure2-1.js
```js
module.exports = function({ $f }) {
module.exports = function({ $Foo }) {
class $Closure2 {
constructor({ }) {
const $obj = (...args) => this.handle(...args);
Object.setPrototypeOf($obj, this);
return $obj;
}
async handle() {
(await $f.print("hey there"));
(await $Foo.print("hey there"));
}
}
return $Closure2;
Expand All @@ -51,7 +51,7 @@ module.exports = function({ }) {
static async getData() {
return (require("<ABSOLUTE_PATH>/external_js.js")["getData"])()
}
async print(msg) {
static async print(msg) {
return (require("<ABSOLUTE_PATH>/external_js.js")["print"])(msg)
}
async call() {
Expand Down Expand Up @@ -348,7 +348,7 @@ class $Root extends $stdlib.std.Resource {
static _toInflightType(context) {
return `
require("./inflight.$Closure2-1.js")({
$f: ${context._lift(f)},
$Foo: ${context._lift(Foo)},
})
`;
}
Expand All @@ -368,7 +368,7 @@ class $Root extends $stdlib.std.Resource {
}
_registerBind(host, ops) {
if (ops.includes("handle")) {
$Closure2._registerBindObject(f, host, ["print"]);
$Closure2._registerBindObject(Foo, host, ["print"]);
}
super._registerBind(host, ops);
}
Expand Down

0 comments on commit ca6d5be

Please sign in to comment.