Skip to content

Commit

Permalink
fix(compiler): panic when method has same name as a field (#5666)
Browse files Browse the repository at this point in the history 
Fixes #1635

Fixing this required a bit of refactoring code around because even when there are member name conflicts, we want to make sure all of the code in the method still gets type checked. Type checking the body requires knowing the type signature of the method. But the method's correct type isn't available in the environment because of the name conflicts.

To solve this, I added some logic to keep track of a mapping of method name symbols to the method types within `type_check_class` (where `type_check_method` is called from). Since the keys of the map are symbols, and each symbol has a name and source location, the method types won't overwrite each other.

## 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
Chriscbr authored Feb 12, 2024
1 parent 545ed6e commit 89c1d55
Show file tree
Hide file tree
Showing 3 changed files with 197 additions and 23 deletions.
53 changes: 52 additions & 1 deletion examples/tests/invalid/class.test.w
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,4 +163,55 @@ inflight class Jet extends Plane{
}

new std.Resource();
//^^^^^^^^^^^^^^^^ Cannot instantiate abstract class "Resource"
//^^^^^^^^^^^^^^^^ Cannot instantiate abstract class "Resource"

// Class with field and method of the same name (field comes first)
class C12 {
z: num;
z(): num {
//^ Symbol "z" is already defined
// The method body should still be type checked
2 + "2";
//^ Expected type to be "num", but got "str" instead
this.z == 5; // OK
return "hello";
//^ Expected type to be "num", but got "str" instead
}
new() {
this.z = 5;
}
z: str;
//^ Symbol "z" is already defined
z(): str {
//^ Symbol "z" is already defined
2 + "2";
//^ Expected type to be "num", but got "str" instead
return 5;
//^ Expected type to be "str", but got "num" instead
}
}

// Class with method and field of the same name (method comes first)
class C13 {
z(): num {
//^ Symbol "z" is already defined
// The method body should still be type checked
2 + "2";
//^ Expected type to be "num", but got "str" instead
return "hello";
//^ Expected type to be "num", but got "str" instead
}
z: num;
new() {
this.z = 5;
}
z(): str {
//^ Symbol "z" is already defined
2 + "2";
//^ Expected type to be "num", but got "str" instead
return 5;
//^ Expected type to be "str", but got "num" instead
}
z: str;
//^ Symbol "z" is already defined
}
52 changes: 30 additions & 22 deletions libs/wingc/src/type_check.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ use itertools::{izip, Itertools};
use jsii_importer::JsiiImporter;

use std::cmp;
use std::collections::{HashMap, HashSet};
use std::collections::{BTreeMap, HashMap, HashSet};
use std::fmt::{Debug, Display};
use std::iter::FilterMap;
use symbol_env::{StatementIdx, SymbolEnv};
Expand Down Expand Up @@ -3985,15 +3985,17 @@ impl<'a> TypeChecker<'a> {
}

// Add methods to the class env
let mut method_types: BTreeMap<&Symbol, TypeRef> = BTreeMap::new();
for (method_name, method_def) in ast_class.methods.iter() {
let mut method_type = self.resolve_type_annotation(&method_def.signature.to_type_annotation(), env);
self.add_method_to_class_env(
&method_def.signature,
env,
&mut method_type,
if method_def.is_static { None } else { Some(class_type) },
method_def.access,
&mut class_env,
method_name,
);
method_types.insert(&method_name, method_type);
}

// Add the constructor to the class env
Expand All @@ -4002,29 +4004,32 @@ impl<'a> TypeChecker<'a> {
span: ast_class.initializer.span.clone(),
};

let mut init_func_type = self.resolve_type_annotation(&ast_class.initializer.signature.to_type_annotation(), env);
self.add_method_to_class_env(
&ast_class.initializer.signature,
env,
&mut init_func_type,
None,
ast_class.initializer.access,
&mut class_env,
&init_symb,
);
method_types.insert(&init_symb, init_func_type);

let inflight_init_symb = Symbol {
name: CLASS_INFLIGHT_INIT_NAME.into(),
span: ast_class.inflight_initializer.span.clone(),
};

// Add the inflight initializer to the class env
let mut inflight_init_func_type =
self.resolve_type_annotation(&ast_class.inflight_initializer.signature.to_type_annotation(), env);
self.add_method_to_class_env(
&ast_class.inflight_initializer.signature,
env,
&mut inflight_init_func_type,
Some(class_type),
ast_class.inflight_initializer.access,
&mut class_env,
&inflight_init_symb,
);
method_types.insert(&inflight_init_symb, inflight_init_func_type);

// Replace the dummy class environment with the real one before type checking the methods
if let Some(mut_class) = class_type.as_class_mut() {
Expand All @@ -4038,7 +4043,14 @@ impl<'a> TypeChecker<'a> {
};

// Type check constructor
self.type_check_method(class_type, &init_symb, env, stmt.idx, &ast_class.initializer);
self.type_check_method(
class_type,
&init_symb,
&mut init_func_type,
env,
stmt.idx,
&ast_class.initializer,
);

// Verify if all fields of a class/resource are initialized in the initializer.
let init_statements = match &ast_class.initializer.body {
Expand All @@ -4052,6 +4064,7 @@ impl<'a> TypeChecker<'a> {
self.type_check_method(
class_type,
&inflight_init_symb,
&mut inflight_init_func_type,
env,
stmt.idx,
&ast_class.inflight_initializer,
Expand All @@ -4062,7 +4075,8 @@ impl<'a> TypeChecker<'a> {

// Type check methods
for (method_name, method_def) in ast_class.methods.iter() {
self.type_check_method(class_type, method_name, env, stmt.idx, method_def);
let mut method_type = *method_types.get(&method_name).unwrap();
self.type_check_method(class_type, method_name, &mut method_type, env, stmt.idx, method_def);
}

// Check that the class satisfies all of its interfaces
Expand Down Expand Up @@ -4696,20 +4710,14 @@ impl<'a> TypeChecker<'a> {
&mut self,
class_type: TypeRef,
method_name: &Symbol,
method_type: &mut TypeRef,
parent_env: &SymbolEnv, // the environment in which the class is declared
statement_idx: usize,
method_def: &FunctionDefinition,
) {
let class_env = &class_type.as_class().unwrap().env;
// TODO: make sure this function returns on all control paths when there's a return type (can be done by recursively traversing the statements and making sure there's a "return" statements in all control paths)
// https://github.com/winglang/wing/issues/457
// Lookup the method in the class_env
let method_type = class_env
.lookup(&method_name, None)
.expect(format!("Expected method '{}' to be in class env", method_name.name).as_str())
.as_variable()
.expect("Expected method to be a variable")
.type_;

let method_sig = method_type
.as_function_sig()
Expand All @@ -4721,7 +4729,7 @@ impl<'a> TypeChecker<'a> {
Some(parent_env.get_ref()),
SymbolEnvKind::Function {
is_init,
sig: method_type,
sig: *method_type,
},
method_sig.phase,
statement_idx,
Expand Down Expand Up @@ -4767,14 +4775,12 @@ impl<'a> TypeChecker<'a> {

fn add_method_to_class_env(
&mut self,
method_sig: &ast::FunctionSignature,
env: &mut SymbolEnv,
method_type: &mut TypeRef,
instance_type: Option<TypeRef>,
access: AccessModifier,
class_env: &mut SymbolEnv,
method_name: &Symbol,
) {
let mut method_type = self.resolve_type_annotation(&method_sig.to_type_annotation(), env);
// use the class type as the function's "this" type (or None if static)
method_type
.as_mut_function_sig()
Expand Down Expand Up @@ -4815,14 +4821,16 @@ impl<'a> TypeChecker<'a> {
}
}

let method_phase = method_type.as_function_sig().unwrap().phase;

match class_env.define(
method_name,
SymbolKind::make_member_variable(
method_name.clone(),
method_type,
*method_type,
false,
instance_type.is_none(),
method_sig.phase,
method_phase,
access,
None,
),
Expand Down
115 changes: 115 additions & 0 deletions tools/hangar/__snapshots__/invalid.ts.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -887,6 +887,65 @@ error: Cannot instantiate abstract class \\"Resource\\"
| ^^^^^^^^^^^^^^^^^^ Cannot instantiate abstract class \\"Resource\\"


error: Symbol \\"z\\" already defined in this scope
--> ../../../examples/tests/invalid/class.test.w:183:3
|
170 | z: num;
| - previous definition
.
183 | z: str;
| ^ Symbol \\"z\\" already defined in this scope


error: Symbol \\"z\\" already defined in this scope
--> ../../../examples/tests/invalid/class.test.w:171:3
|
170 | z: num;
| - previous definition
171 | z(): num {
| ^ Symbol \\"z\\" already defined in this scope


error: Symbol \\"z\\" already defined in this scope
--> ../../../examples/tests/invalid/class.test.w:185:3
|
170 | z: num;
| - previous definition
.
185 | z(): str {
| ^ Symbol \\"z\\" already defined in this scope


error: Symbol \\"z\\" already defined in this scope
--> ../../../examples/tests/invalid/class.test.w:215:3
|
204 | z: num;
| - previous definition
.
215 | z: str;
| ^ Symbol \\"z\\" already defined in this scope


error: Symbol \\"z\\" already defined in this scope
--> ../../../examples/tests/invalid/class.test.w:196:3
|
196 | z(): num {
| ^ Symbol \\"z\\" already defined in this scope
.
204 | z: num;
| - previous definition


error: Symbol \\"z\\" already defined in this scope
--> ../../../examples/tests/invalid/class.test.w:208:3
|
204 | z: num;
| - previous definition
.
208 | z(): str {
| ^ Symbol \\"z\\" already defined in this scope


error: Expected type to be \\"num\\", but got \\"str\\" instead
--> ../../../examples/tests/invalid/class.test.w:31:14
|
Expand Down Expand Up @@ -929,6 +988,62 @@ error: Expected 1 positional argument(s) but got 0
| ^^^^^^^^ Expected 1 positional argument(s) but got 0


error: Binary operator '+' cannot be applied to operands of type 'num' and 'str'; only (num, num) and (str, str) are supported
--> ../../../examples/tests/invalid/class.test.w:174:5
|
174 | 2 + \\"2\\";
| ^^^^^^^ Binary operator '+' cannot be applied to operands of type 'num' and 'str'; only (num, num) and (str, str) are supported


error: Expected type to be \\"num\\", but got \\"str\\" instead
--> ../../../examples/tests/invalid/class.test.w:177:12
|
177 | return \\"hello\\";
| ^^^^^^^ Expected type to be \\"num\\", but got \\"str\\" instead


error: Binary operator '+' cannot be applied to operands of type 'num' and 'str'; only (num, num) and (str, str) are supported
--> ../../../examples/tests/invalid/class.test.w:187:5
|
187 | 2 + \\"2\\";
| ^^^^^^^ Binary operator '+' cannot be applied to operands of type 'num' and 'str'; only (num, num) and (str, str) are supported


error: Expected type to be \\"str\\", but got \\"num\\" instead
--> ../../../examples/tests/invalid/class.test.w:189:12
|
189 | return 5;
| ^ Expected type to be \\"str\\", but got \\"num\\" instead


error: Binary operator '+' cannot be applied to operands of type 'num' and 'str'; only (num, num) and (str, str) are supported
--> ../../../examples/tests/invalid/class.test.w:199:5
|
199 | 2 + \\"2\\";
| ^^^^^^^ Binary operator '+' cannot be applied to operands of type 'num' and 'str'; only (num, num) and (str, str) are supported


error: Expected type to be \\"num\\", but got \\"str\\" instead
--> ../../../examples/tests/invalid/class.test.w:201:12
|
201 | return \\"hello\\";
| ^^^^^^^ Expected type to be \\"num\\", but got \\"str\\" instead


error: Binary operator '+' cannot be applied to operands of type 'num' and 'str'; only (num, num) and (str, str) are supported
--> ../../../examples/tests/invalid/class.test.w:210:5
|
210 | 2 + \\"2\\";
| ^^^^^^^ Binary operator '+' cannot be applied to operands of type 'num' and 'str'; only (num, num) and (str, str) are supported


error: Expected type to be \\"str\\", but got \\"num\\" instead
--> ../../../examples/tests/invalid/class.test.w:212:12
|
212 | return 5;
| ^ Expected type to be \\"str\\", but got \\"num\\" instead




Tests 1 failed (1)
Expand Down

0 comments on commit 89c1d55

Please sign in to comment.