Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(compiler): lifting globals from inflight classes defined in preflight fails #5559

Open
wants to merge 77 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
77 commits
Select commit Hold shift + click to select a range
2ec7bb9
wip
yoav-steinberg Jan 28, 2024
8e9de7d
wip
yoav-steinberg Jan 28, 2024
33f8765
Merge branch 'main' into yoav/qualify_err_closure_lift
monadabot Jan 28, 2024
6b595d7
chore: self mutation (build.diff)
monadabot Jan 28, 2024
10f0670
store whether expr is callee in AST instead of during visiting
yoav-steinberg Jan 28, 2024
6a366e4
Merge branch 'yoav/qualify_err_closure_lift' of https://github.com/wi…
yoav-steinberg Jan 28, 2024
f5a6ac4
wip: lift inflight classes (defined preflight)
yoav-steinberg Feb 1, 2024
97d10f5
deduce class phase in type system based on class definitions
yoav-steinberg Feb 3, 2024
4e82c45
better handling of lifting host agnostic types
yoav-steinberg Feb 12, 2024
5f7ddb6
correctly handle `onLift` of synthetic resource type generated by `to…
yoav-steinberg Feb 12, 2024
3942817
treat class phase based on code modifier (chuck auto-phase idea for now)
yoav-steinberg Feb 12, 2024
9df1062
back to lifting all preflight expressions referencing inflight closures
yoav-steinberg Feb 12, 2024
36b89b8
visit all udt refs before qualifying inflight type lifts
yoav-steinberg Feb 12, 2024
d3ef0cc
Qualify inflight class usage even when type not explicitly lifted
yoav-steinberg Feb 16, 2024
f8dd462
test qualify inflight type when not available in scope
yoav-steinberg Feb 16, 2024
cb94d46
test
yoav-steinberg Feb 16, 2024
b833982
cleanup
yoav-steinberg Feb 16, 2024
9fee686
generate less class aliases (smaller snapshot diff)
yoav-steinberg Feb 16, 2024
463568c
added test
yoav-steinberg Feb 16, 2024
916b54f
typo
yoav-steinberg Feb 16, 2024
0b149b6
test static method lifts of inflight class
yoav-steinberg Feb 16, 2024
7dc5856
Merge branch 'main' into yoav/qualify_err_closure_lift
yoav-steinberg Feb 17, 2024
48ecb71
fix after merge
yoav-steinberg Feb 17, 2024
12b54e2
Merge branch 'main' into yoav/qualify_err_closure_lift
monadabot Feb 17, 2024
c8c8678
chore: self mutation (e2e-1of2.diff)
monadabot Feb 17, 2024
118510e
chore: self mutation (e2e-2of2.diff)
monadabot Feb 17, 2024
2d436f0
lint
yoav-steinberg Feb 17, 2024
cb749b8
Merge branch 'yoav/qualify_err_closure_lift' of https://github.com/wi…
yoav-steinberg Feb 17, 2024
e3dd5d0
Merge branch 'main' into yoav/qualify_err_closure_lift
monadabot Feb 17, 2024
d6f0242
chore: self mutation (build.diff)
monadabot Feb 17, 2024
eb9205b
No need to store TypeRef in lift qualifications (we use unique type n…
yoav-steinberg Feb 18, 2024
4e865a6
use dummy object lift map for inflight class isntead of type lift map
yoav-steinberg Feb 20, 2024
e9c883c
Merge branch 'main' into yoav/qualify_err_closure_lift
yoav-steinberg Feb 20, 2024
7a45ea8
fix after merge
yoav-steinberg Feb 20, 2024
0f0b8cd
remove leftover of experiment generating inflight_init lifitng map fo…
yoav-steinberg Feb 20, 2024
e98b310
Merge branch 'main' into yoav/qualify_err_closure_lift
monadabot Feb 20, 2024
2dbf12a
chore: self mutation (build.diff)
monadabot Feb 20, 2024
fd71b88
chore: self mutation (e2e-1of2.diff)
monadabot Feb 20, 2024
0db70d9
chore: self mutation (e2e-2of2.diff)
monadabot Feb 20, 2024
8e90a4f
inflight class singletons are defined per root node and not globaly
yoav-steinberg Mar 10, 2024
cdf4b66
Merge branch 'yoav/qualify_err_closure_lift' of https://github.com/wi…
yoav-steinberg Mar 10, 2024
69deae1
cleanup
yoav-steinberg Jun 8, 2024
138a4b0
wip
yoav-steinberg Jun 8, 2024
0baf197
wip
yoav-steinberg Jun 11, 2024
e498b9e
wip
yoav-steinberg Jun 12, 2024
0e48d9a
modules are imported into root ctor
yoav-steinberg Jun 12, 2024
0032c4a
cleanup
yoav-steinberg Jun 12, 2024
edc306c
Merge branch 'main' into yoav/qualify_err_closure_lift
yoav-steinberg Jun 13, 2024
7db9384
fixes after merge
yoav-steinberg Jun 13, 2024
a3f4592
more fixes after merge
yoav-steinberg Jun 13, 2024
5729cb5
lint
yoav-steinberg Jun 13, 2024
a977178
Merge branch 'main' into yoav/qualify_err_closure_lift
monadabot Jun 13, 2024
f3f0f10
chore: self mutation (e2e-1of2.diff)
monadabot Jun 13, 2024
e9f49fe
chore: self mutation (e2e-2of2.diff)
monadabot Jun 13, 2024
2a7fc0e
lint
yoav-steinberg Jun 13, 2024
c3ec11e
Merge branch 'yoav/qualify_err_closure_lift' of https://github.com/wi…
yoav-steinberg Jun 13, 2024
46539c0
Merge branch 'main' into yoav/qualify_err_closure_lift
monadabot Jun 13, 2024
76ac1a1
chore: self mutation (build.diff)
monadabot Jun 13, 2024
6783e2f
tests
yoav-steinberg Jun 13, 2024
b38f857
Merge branch 'yoav/qualify_err_closure_lift' of https://github.com/wi…
yoav-steinberg Jun 13, 2024
4747daa
cleanup
yoav-steinberg Jun 13, 2024
c078927
Merge branch 'main' into yoav/qualify_err_closure_lift
monadabot Jun 13, 2024
7e6b6ce
chore: self mutation (build.diff)
monadabot Jun 13, 2024
90521a6
chore: self mutation (e2e-1of2.diff)
monadabot Jun 13, 2024
dc0b7d7
chore: self mutation (e2e-2of2.diff)
monadabot Jun 13, 2024
2fd723e
cleanup
yoav-steinberg Jun 13, 2024
d521a41
Merge branch 'yoav/qualify_err_closure_lift' of https://github.com/wi…
yoav-steinberg Jun 13, 2024
226e2b2
cleanup
yoav-steinberg Jun 13, 2024
ac1c19b
Merge branch 'main' into yoav/qualify_err_closure_lift
monadabot Jun 13, 2024
e7ff30f
chore: self mutation (build.diff)
monadabot Jun 13, 2024
41d93dd
cleanup
yoav-steinberg Jun 13, 2024
704dd47
comment
yoav-steinberg Jun 13, 2024
82da9d0
Merge branch 'yoav/qualify_err_closure_lift' of https://github.com/wi…
yoav-steinberg Jun 13, 2024
e0d4adc
don't use global for class types ids (causes non consistent snaps)
yoav-steinberg Jun 16, 2024
f161b99
cleanup, merge fix
yoav-steinberg Jun 16, 2024
2b7e63e
cr, use `$helpers.bringJs` in for all imports, document it
yoav-steinberg Jun 17, 2024
36fbbac
cr cleanup
yoav-steinberg Jun 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion libs/wingc/src/docs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ fn render_class(c: &Class) -> String {
}
render_docs(&mut markdown, &c.docs);

if matches!(c.phase, Phase::Preflight | Phase::Independent) {
if matches!(c.phase(), Phase::Preflight | Phase::Independent) {
if let Some(initializer) = c.get_method(&Symbol::global(CLASS_INIT_NAME)) {
let function_sig = initializer.type_.as_function_sig().unwrap();
if function_sig.parameters.len() > 0 {
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 @@ -1902,7 +1902,7 @@ fn parent_class_phase(ctx: &JSifyContext<'_>) -> Phase {
.expect("a parent class")
.as_class()
.expect("a class")
.phase;
.phase();
parent_class_phase
}

Expand Down
2 changes: 1 addition & 1 deletion libs/wingc/src/lifting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ impl<'a> Visit<'a> for LiftVisitor<'a> {
if let Some(class) = expr_type.as_class() {
//let tmp_udt = UserDefinedType::for_name(&class.name);
//let class_defined_in = get_udt_definition_phase(&tmp_udt, v.ctx.current_env().expect("an env")).expect("a phase");
if class.phase == Phase::Inflight && class.defined_in_phase == Phase::Preflight {
if class.phase() == Phase::Inflight && class.defined_in_phase == Phase::Preflight {
if let Some(property) = v.ctx.current_property() {
let m = v.ctx.current_method().map(|(m,_)|m).expect("a method");
//println!("Access to {property} of preflight defined inflight class {expr_type}, should qualify method {m}!");
Expand Down
128 changes: 71 additions & 57 deletions libs/wingc/src/type_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,6 @@ pub struct Class {
pub fqn: Option<String>,
pub is_abstract: bool,
pub type_parameters: Option<Vec<TypeRef>>,
pub phase: Phase,
pub docs: Docs,
pub lifts: Option<Lifts>,
pub defined_in_phase: Phase, // TODO: naming: maybe this should just be `phase` while `phase` should be `defauld_member_phase`?
Expand All @@ -345,8 +344,8 @@ impl Class {
pub fn get_closure_method(&self) -> Option<TypeRef> {
self
.methods(true)
.find(|(name, type_)| name == CLOSURE_CLASS_HANDLE_METHOD && type_.is_inflight_function())
.map(|(_, t)| t)
.find(|(name, v)| name == CLOSURE_CLASS_HANDLE_METHOD && v.type_.is_inflight_function())
.map(|(_, v)| v.type_)
}
}

Expand Down Expand Up @@ -391,25 +390,23 @@ impl Display for Interface {
}

type ClassLikeIterator<'a> =
FilterMap<SymbolEnvIter<'a>, fn(<SymbolEnvIter as Iterator>::Item) -> Option<(String, TypeRef)>>;
FilterMap<SymbolEnvIter<'a>, fn(<SymbolEnvIter as Iterator>::Item) -> Option<(String, VariableInfo)>>;

pub trait ClassLike: Display {
fn get_env(&self) -> &SymbolEnv;

fn methods(&self, with_ancestry: bool) -> ClassLikeIterator<'_> {
self.get_env().iter(with_ancestry).filter_map(|(s, t, ..)| {
t.as_variable()
.unwrap()
.type_
.as_function_sig()
.map(|_| (s.clone(), t.as_variable().unwrap().type_))
self.get_env().iter(with_ancestry).filter_map(|(s, sym_kind, ..)| {
let v = sym_kind.as_variable().unwrap();
v.type_.as_function_sig().map(|_| (s.clone(), v.clone()))
})
}

fn fields(&self, with_ancestry: bool) -> ClassLikeIterator<'_> {
self.get_env().iter(with_ancestry).filter_map(|(s, t, ..)| {
if t.as_variable().unwrap().type_.as_function_sig().is_none() {
Some((s, t.as_variable().unwrap().type_))
self.get_env().iter(with_ancestry).filter_map(|(s, sym_kind, ..)| {
let v = sym_kind.as_variable().unwrap();
if v.type_.as_function_sig().is_none() {
Some((s, v.clone()))
} else {
None
}
Expand Down Expand Up @@ -445,6 +442,32 @@ pub trait ClassLike: Display {
None
}
}

fn phase(&self) -> Phase {
// A class is considered a "preflight class" if it has any preflight fields.
// Having a preflight state means that different instances of the class
// might access different preflight data. Such instances need to be lifted
// into inflight individually. Non preflight class instances don't need to be
// lifted and can therefore be instantiated inflight.
if self
.fields(true)
.any(|(_, vi)| vi.kind == VariableKind::InstanceMember && vi.phase == Phase::Preflight)
{
return Phase::Preflight;
}
yoav-steinberg marked this conversation as resolved.
Show resolved Hide resolved

// If all memebers are phase independent, then this class is phase independent
if self
.get_env()
.iter(true)
.all(|(_, v, _)| v.as_variable().unwrap().phase == Phase::Independent)
{
return Phase::Independent;
}

// Otherwise, this is an inflight class
Phase::Inflight
}
}

impl ClassLike for Interface {
Expand Down Expand Up @@ -659,16 +682,14 @@ impl Subtype for Type {
// method type (aka "closure classes").

// First, check if there is exactly one inflight method in the interface
let mut inflight_methods = iface
.methods(true)
.filter(|(_name, type_)| type_.is_inflight_function());
let mut inflight_methods = iface.methods(true).filter(|(_name, v)| v.type_.is_inflight_function());
let handler_method = inflight_methods.next();
if handler_method.is_none() || inflight_methods.next().is_some() {
return false;
}

// Next, check that the method's name is "handle"
let (handler_method_name, handler_method_type) = handler_method.unwrap();
let (handler_method_name, handler_method_var) = handler_method.unwrap();
if handler_method_name != CLOSURE_CLASS_HANDLE_METHOD {
return false;
}
Expand All @@ -681,7 +702,7 @@ impl Subtype for Type {
};

// Finally check if they're subtypes
res_handle_type.is_subtype_of(&handler_method_type)
res_handle_type.is_subtype_of(&handler_method_var.type_)
}
(Self::Class(res), Self::Function(_)) => {
// To support flexible inflight closures, we say that any
Expand Down Expand Up @@ -929,7 +950,7 @@ unsafe impl Send for TypeRef {}
impl TypeRef {
pub fn as_preflight_class(&self) -> Option<&Class> {
if let Type::Class(ref class) = **self {
if class.phase == Phase::Preflight {
if class.phase() == Phase::Preflight {
return Some(class);
}
}
Expand Down Expand Up @@ -1053,7 +1074,7 @@ impl TypeRef {

pub fn is_preflight_class(&self) -> bool {
if let Type::Class(ref class) = **self {
return class.phase == Phase::Preflight;
return class.phase() == Phase::Preflight;
}

return false;
Expand All @@ -1064,10 +1085,10 @@ impl TypeRef {
self.as_function_sig().is_some() || self.is_closure_class()
}

/// Returns whether type represents a preflight class representing and inflight closure.
/// Returns whether type represents a class representing an inflight closure.
pub fn is_closure_class(&self) -> bool {
if let Some(ref class) = self.as_preflight_class() {
return class.get_closure_method().is_some();
if let Some(ref class) = self.as_class() {
return class.get_closure_method().is_some() && class.defined_in_phase == Phase::Preflight;
}
false
}
Expand Down Expand Up @@ -1171,7 +1192,7 @@ impl TypeRef {
Type::Function(sig) => sig.phase == Phase::Inflight,

// only preflight classes can be captured
Type::Class(c) => c.phase == Phase::Preflight,
Type::Class(c) => c.phase() == Phase::Preflight,
}
}

Expand Down Expand Up @@ -1214,7 +1235,7 @@ impl TypeRef {
Type::Map(v) => v.is_json_legal_value(),
Type::Optional(v) => v.is_json_legal_value(),
Type::Struct(ref s) => {
for (_, t) in s.fields(true) {
for t in s.fields(true).map(|(_, v)| v.type_) {
if !t.is_json_legal_value() {
return false;
}
Expand Down Expand Up @@ -1251,8 +1272,8 @@ impl TypeRef {
match &**self {
Type::Struct(s) => {
// check all its fields are json compatible
for (_, field) in s.fields(true) {
if !field.has_json_representation() {
for t in s.fields(true).map(|(_, v)| v.type_) {
if !t.has_json_representation() {
return false;
}
}
Expand Down Expand Up @@ -2155,7 +2176,7 @@ impl<'a> TypeChecker<'a> {

// error if we are trying to instantiate a preflight in a static method
// without an explicit scope (there is no "this" to use as the scope)
if class.phase == Phase::Preflight && obj_scope.is_none() {
if class.phase() == Phase::Preflight && obj_scope.is_none() {
// check if there is a "this" symbol in the current environment
let has_this = env.lookup(&"this".into(), Some(self.ctx.current_stmt_idx())).is_some();

Expand All @@ -2176,14 +2197,16 @@ impl<'a> TypeChecker<'a> {
}
}

if class.phase == Phase::Independent || env.phase == class.phase {
if class.phase() == Phase::Independent || env.phase == class.phase() {
(&class.env, &class.name)
} else {
self.spanned_error(
exp,
format!(
"Cannot create {} class \"{}\" in {} phase",
class.phase, class.name, env.phase
class.phase(),
class.name,
env.phase
),
);
return (self.types.error(), Phase::Independent);
Expand Down Expand Up @@ -2339,21 +2362,9 @@ impl<'a> TypeChecker<'a> {
// Make sure this is a function signature type
let func_sig = if let Some(func_sig) = func_type.as_function_sig() {
func_sig.clone()
} else if let Some(class) = func_type.as_preflight_class() {
// return the signature of the "handle" method
let lookup_res = class.get_method(&CLOSURE_CLASS_HANDLE_METHOD.into());
let handle_type = if let Some(method) = lookup_res {
method.type_
} else {
self.spanned_error(callee, "Expected a function or method");
return self.resolved_error();
};
if let Some(sig_type) = handle_type.as_function_sig() {
sig_type.clone()
} else {
self.spanned_error(callee, "Expected a function or method");
return self.resolved_error();
}
} else if func_type.is_closure_class() {
let handle_type = func_type.as_class().unwrap().get_closure_method().unwrap();
handle_type.as_function_sig().unwrap().clone()
} else {
self.spanned_error(
callee,
Expand Down Expand Up @@ -2608,7 +2619,8 @@ impl<'a> TypeChecker<'a> {
.expect(&format!("Expected \"{}\" to be a struct type", struct_type));

// Verify that all expected fields are present and are the right type
for (name, field_type) in st.fields(true) {
for (name, v) in st.fields(true) {
let field_type = v.type_;
match fields.get(name.as_str()) {
Some(field_exp) => {
let t = field_types.get(name.as_str()).unwrap();
Expand Down Expand Up @@ -3877,7 +3889,6 @@ impl<'a> TypeChecker<'a> {
parent: parent_class,
implements: impl_interfaces.clone(),
is_abstract: false,
phase: ast_class.phase,
defined_in_phase: env.phase,
type_parameters: None, // TODO no way to have generic args in wing yet
docs: Docs::default(),
Expand Down Expand Up @@ -4016,7 +4027,8 @@ impl<'a> TypeChecker<'a> {
};

// Check all methods are implemented
for (method_name, method_type) in interface_type.methods(true) {
for (method_name, v) in interface_type.methods(true) {
let method_type = v.type_;
if let Some(symbol) = &mut class_type
.as_class_mut()
.unwrap()
Expand Down Expand Up @@ -4561,9 +4573,9 @@ 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 then its init name is just "init" regadless of
// whether we're inflight or not.
let parent_init_name = if parent_class.as_class().unwrap().phase == Phase::Independent {
let parent_init_name = if parent_class.as_class().unwrap().phase() == Phase::Independent {
CLASS_INIT_NAME
} else {
init_name
Expand All @@ -4573,13 +4585,13 @@ impl<'a> TypeChecker<'a> {
.as_class()
.unwrap()
.methods(false)
.filter(|(name, _type)| name == parent_init_name)
.collect_vec()[0]
.find(|(name, ..)| name == parent_init_name)
.unwrap()
.1;

self.type_check_arg_list_against_function_sig(
&arg_list,
parent_initializer.as_function_sig().unwrap(),
parent_initializer.type_.as_function_sig().unwrap(),
super_constructor_call,
arg_list_types,
);
Expand Down Expand Up @@ -4950,7 +4962,6 @@ impl<'a> TypeChecker<'a> {
implements: original_type_class.implements.clone(),
is_abstract: original_type_class.is_abstract,
type_parameters: Some(type_params),
phase: original_type_class.phase,
defined_in_phase: env.phase,
docs: original_type_class.docs.clone(),
std_construct_args: original_type_class.std_construct_args,
Expand Down Expand Up @@ -5396,7 +5407,7 @@ impl<'a> TypeChecker<'a> {
if property.name == FROM_JSON || property.name == TRY_FROM_JSON {
// we need to validate that only structs with all valid json fields can have a fromJson method
for (name, field) in s.fields(true) {
if !field.has_json_representation() {
if !field.type_.has_json_representation() {
self.spanned_error_with_var(
property,
format!(
Expand Down Expand Up @@ -5690,14 +5701,17 @@ impl<'a> TypeChecker<'a> {

if let Some(parent_class) = parent_type.as_class() {
// Parent class must be either the same phase as the child or, if the child is an inflight class, the parent can be an independent class
if (parent_class.phase == phase) || (phase == Phase::Inflight && parent_class.phase == Phase::Independent) {
if (parent_class.phase() == phase) || (phase == Phase::Inflight && parent_class.phase() == Phase::Independent) {
(Some(parent_type), Some(parent_class.env.get_ref()))
} else {
self.spanned_error(
parent,
format!(
"Class \"{}\" is an {} class and cannot extend {} class \"{}\"",
name, phase, parent_class.phase, parent_class.name
name,
phase,
parent_class.phase(),
parent_class.name
),
);
(None, None)
Expand Down
7 changes: 3 additions & 4 deletions libs/wingc/src/type_check/jsii_importer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ use crate::{
type_check::{
self,
symbol_env::{StatementIdx, SymbolEnvKind},
Class, FunctionParameter, FunctionSignature, Interface, ResolveSource, Struct, SymbolKind, Type, TypeRef, Types,
CLASS_INIT_NAME,
Class, ClassLike, FunctionParameter, FunctionSignature, Interface, ResolveSource, Struct, SymbolKind, Type,
TypeRef, Types, CLASS_INIT_NAME,
},
CONSTRUCT_BASE_CLASS, WINGSDK_ASSEMBLY_NAME, WINGSDK_DURATION, WINGSDK_JSON, WINGSDK_MUT_JSON, WINGSDK_RESOURCE,
};
Expand Down Expand Up @@ -655,7 +655,7 @@ impl<'a> JsiiImporter<'a> {
"Base class {} of {} is not a class",
base_class_type, type_name
));
class_phase = base_class.phase;
class_phase = base_class.phase();
Some(base_class.env.get_ref())
} else {
None
Expand Down Expand Up @@ -701,7 +701,6 @@ impl<'a> JsiiImporter<'a> {
implements: vec![],
is_abstract: jsii_class.abstract_.unwrap_or(false),
type_parameters: type_params,
phase: class_phase,
defined_in_phase: Phase::Preflight,
docs: Docs::from(&jsii_class.docs),
std_construct_args: false, // Temporary value, will be updated once we parse the initializer args
Expand Down