Skip to content

Commit

Permalink
fix(compiler): confusing error when trying to use private constructor (
Browse files Browse the repository at this point in the history
…#6242)

Fixes #3331

Besides updating this error message, I also updated other error messages to standardize around the name "constructor". This is the name used most in the language reference, and it's probably the most familiar to users coming from TypeScript, so sticking to one terminology in our errors should help make the language feel more cohesive.

## 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 Apr 15, 2024
1 parent 9776284 commit 97a205a
Show file tree
Hide file tree
Showing 9 changed files with 79 additions and 40 deletions.
2 changes: 1 addition & 1 deletion docs/docs/03-language-reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,7 @@ let handler2 = inflight() => {
Bridge between preflight and inflight is crossed with the help of immutable data
structures, "structs" (user definable and `Struct`), and the capture mechanism.
Preflight class methods and initializers can receive an inflight function as an argument. This
Preflight class methods and constructors can receive an inflight function as an argument. This
enables preflight classes to define code that will be executed on a cloud compute platform such as
lambda functions, docker, virtual machines etc.
Expand Down
10 changes: 9 additions & 1 deletion examples/jsii-fixture/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@ export class JsiiClass {
}
}

export class JsiiClassWithPrivateConstructor {
private constructor() {}

public static makeInstance(): JsiiClassWithPrivateConstructor {
return new JsiiClassWithPrivateConstructor();
}
}

/**
* @callable
*/
Expand All @@ -47,4 +55,4 @@ export interface SomeStruct {

export interface ISomeInterface {
method(): void;
}
}
2 changes: 1 addition & 1 deletion examples/tests/invalid/class.test.w
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ inflight class Jet extends Plane{
// ^^^^^^^^ Expected 1 positional argument(s) but got 0
}
constructor() {
//^^^^^^^^^^^ To declare a initializer, use "init"
//^^^^^^^^^^^ To declare a constructor, use "new"
}
}

Expand Down
2 changes: 1 addition & 1 deletion examples/tests/invalid/inflight_class_dup_init.test.w
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,5 @@ inflight class Foo {
inflight new() {

}
//^ Multiple inflight initializers defined in class Foo
//^ Multiple inflight constructors defined in class Foo
}
6 changes: 6 additions & 0 deletions examples/tests/invalid/private_constructor.test.w
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
bring "jsii-fixture" as jsii_fixture;

new jsii_fixture.JsiiClassWithPrivateConstructor();
// error: constructor is private and only accessible within class "JsiiClassWithPrivateConstructor"

jsii_fixture.JsiiClassWithPrivateConstructor.makeInstance(); // OK
9 changes: 4 additions & 5 deletions libs/wingc/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@ static RESERVED_WORDS: phf::Set<&'static str> = phf_set! {
"inflight",
"preflight",
"elif",
"init",
"any",
"num",
"str",
Expand Down Expand Up @@ -1228,14 +1227,14 @@ impl<'s> Parser<'s> {
if initializer.is_some() && !is_inflight {
self
.with_error::<Node>(
format!("Multiple initializers defined in class {}", name.name),
format!("Multiple constructors defined in class {}", name.name),
&class_element,
)
.err();
} else if inflight_initializer.is_some() && is_inflight {
self
.with_error::<Node>(
format!("Multiple inflight initializers defined in class {}", name.name),
format!("Multiple inflight constructors defined in class {}", name.name),
&class_element,
)
.err();
Expand All @@ -1244,7 +1243,7 @@ impl<'s> Parser<'s> {
let parameters = self.build_parameter_list(&parameters_node, class_phase, false)?;
if !parameters.is_empty() && is_inflight && class_phase == Phase::Preflight {
self
.with_error::<Node>("Inflight initializers cannot have parameters", &parameters_node)
.with_error::<Node>("Inflight constructors cannot have parameters", &parameters_node)
.err();
}

Expand Down Expand Up @@ -1303,7 +1302,7 @@ impl<'s> Parser<'s> {
for method in &methods {
if method.0.name == "constructor" {
Diagnostic::new(
"Reserved method name. Initializers are declared with \"init\"",
"Reserved method name. Constructors are declared with a method named \"new\"",
&method.0,
)
.report();
Expand Down
39 changes: 25 additions & 14 deletions libs/wingc/src/type_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ pub enum Type {
Enum(Enum),
}

pub const CLASS_INIT_NAME: &'static str = "init";
pub const CLASS_INIT_NAME: &'static str = "new";
pub const CLASS_INFLIGHT_INIT_NAME: &'static str = "$inflight_init";

pub const CLOSURE_CLASS_HANDLE_METHOD: &'static str = "handle";
Expand Down Expand Up @@ -2314,17 +2314,28 @@ impl<'a> TypeChecker<'a> {
};

let lookup_res = class_env.lookup_ext(&init_method_name.into(), None);
let constructor_type = if let LookupResult::Found(k, _) = lookup_res {
k.as_variable().expect("Expected constructor to be a variable").type_
} else {
self.type_error(lookup_result_to_type_error(
lookup_res,
&Symbol {
name: CLASS_INIT_NAME.into(),
span: class_symbol.span.clone(),
},
));
return self.resolved_error();
let constructor_type = match lookup_res {
LookupResult::Found(k, _) => k.as_variable().expect("Expected constructor to be a variable").type_,
LookupResult::NotFound(_, _) => {
self.spanned_error(
exp,
format!("Constructor for class \"{}\" is private", class_symbol.name),
);
return self.resolved_error();
}
LookupResult::NotPublic(_, _)
| LookupResult::MultipleFound
| LookupResult::DefinedLater(_)
| LookupResult::ExpectedNamespace(_) => {
self.type_error(lookup_result_to_type_error(
lookup_res,
&Symbol {
name: CLASS_INIT_NAME.into(),
span: class_symbol.span.clone(),
},
));
return self.resolved_error();
}
};
let constructor_sig = constructor_type
.as_function_sig()
Expand Down Expand Up @@ -4798,7 +4809,7 @@ impl<'a> TypeChecker<'a> {
return;
};

// If the parent class is phase independent than its init name is just "init" regadless of
// If the parent class is phase independent than its constructor name is just "new" regardless of
// whether we're inflight or not.
let parent_init_name = if parent_class.as_class().unwrap().phase == Phase::Independent {
CLASS_INIT_NAME
Expand Down Expand Up @@ -4851,7 +4862,7 @@ impl<'a> TypeChecker<'a> {
self.spanned_error(
matching_field,
format!(
"\"{}\" cannot be initialized in the {} initializer",
"\"{}\" cannot be initialized in the {} constructor",
matching_field.name,
current_phase.to_lowercase()
),
Expand Down
2 changes: 1 addition & 1 deletion libs/wingc/src/type_check/jsii_importer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -773,7 +773,7 @@ impl<'a> JsiiImporter<'a> {
let mut class_env = SymbolEnv::new(base_class_env, SymbolEnvKind::Type(new_type), class_phase, 0);
class_env.type_parameters = self.type_param_from_docs(&jsii_class_fqn, &jsii_class.docs);

// Add constructor to the class environment
// Add the class's constructor to the class environment, if the class has one which is public
let jsii_initializer = jsii_class.initializer.as_ref();
if let Some(initializer) = jsii_initializer {
let mut fn_params = vec![];
Expand Down
47 changes: 31 additions & 16 deletions tools/hangar/__snapshots__/invalid.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -747,11 +747,11 @@ error: Call to super constructor can only be done from within a class constructo
| ^^^^^^^^ Call to super constructor can only be done from within a class constructor


error: Reserved method name. Initializers are declared with \\"init\\"
error: Reserved method name. Constructors are declared with a method named \\"new\\"
--> ../../../examples/tests/invalid/class.test.w:160:3
|
160 | constructor() {
| ^^^^^^^^^^^ Reserved method name. Initializers are declared with \\"init\\"
| ^^^^^^^^^^^ Reserved method name. Constructors are declared with a method named \\"new\\"


error: Expected block
Expand Down Expand Up @@ -838,18 +838,18 @@ error: Inflight field \\"x\\" is not initialized
| ^ Inflight field \\"x\\" is not initialized


error: \\"y\\" cannot be initialized in the inflight initializer
error: \\"y\\" cannot be initialized in the inflight constructor
--> ../../../examples/tests/invalid/class.test.w:61:10
|
61 | this.y = 1;
| ^ \\"y\\" cannot be initialized in the inflight initializer
| ^ \\"y\\" cannot be initialized in the inflight constructor


error: \\"x\\" cannot be initialized in the preflight initializer
error: \\"x\\" cannot be initialized in the preflight constructor
--> ../../../examples/tests/invalid/class.test.w:56:10
|
56 | this.x = 1;
| ^ \\"x\\" cannot be initialized in the preflight initializer
| ^ \\"x\\" cannot be initialized in the preflight constructor


error: Preflight field \\"y\\" is not initialized
Expand Down Expand Up @@ -2259,13 +2259,13 @@ Duration <DURATION>"
`;

exports[`inflight_class_dup_init.test.w 1`] = `
"error: Multiple inflight initializers defined in class Foo
"error: Multiple inflight constructors defined in class Foo
--> ../../../examples/tests/invalid/inflight_class_dup_init.test.w:6:3
|
6 | / inflight new() {
7 | |
8 | | }
| \\\\---^ Multiple inflight initializers defined in class Foo
| \\\\---^ Multiple inflight constructors defined in class Foo



Expand Down Expand Up @@ -3249,6 +3249,21 @@ error: Expected type to be \\"str\\", but got \\"num\\" instead



Tests 1 failed (1)
Snapshots 1 skipped
Test Files 1 failed (1)
Duration <DURATION>"
`;

exports[`private_constructor.test.w 1`] = `
"error: Constructor for class \\"JsiiClassWithPrivateConstructor\\" is private
--> ../../../examples/tests/invalid/private_constructor.test.w:3:1
|
3 | new jsii_fixture.JsiiClassWithPrivateConstructor();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Constructor for class \\"JsiiClassWithPrivateConstructor\\" is private



Tests 1 failed (1)
Snapshots 1 skipped
Test Files 1 failed (1)
Expand Down Expand Up @@ -3529,32 +3544,32 @@ Duration <DURATION>"
`;

exports[`resource_init.test.w 1`] = `
"error: Multiple initializers defined in class R
"error: Multiple constructors defined in class R
--> ../../../examples/tests/invalid/resource_init.test.w:3:3
|
3 | new() {}
| ^^^^^^^^ Multiple initializers defined in class R
| ^^^^^^^^ Multiple constructors defined in class R


error: Multiple inflight initializers defined in class R
error: Multiple inflight constructors defined in class R
--> ../../../examples/tests/invalid/resource_init.test.w:6:3
|
6 | inflight new() {}
| ^^^^^^^^^^^^^^^^^ Multiple inflight initializers defined in class R
| ^^^^^^^^^^^^^^^^^ Multiple inflight constructors defined in class R


error: Multiple inflight initializers defined in class R
error: Multiple inflight constructors defined in class R
--> ../../../examples/tests/invalid/resource_init.test.w:9:3
|
9 | inflight new(x: num) {}
| ^^^^^^^^^^^^^^^^^^^^^^^ Multiple inflight initializers defined in class R
| ^^^^^^^^^^^^^^^^^^^^^^^ Multiple inflight constructors defined in class R


error: Inflight initializers cannot have parameters
error: Inflight constructors cannot have parameters
--> ../../../examples/tests/invalid/resource_init.test.w:9:15
|
9 | inflight new(x: num) {}
| ^^^^^^^^ Inflight initializers cannot have parameters
| ^^^^^^^^ Inflight constructors cannot have parameters



Expand Down

0 comments on commit 97a205a

Please sign in to comment.