[DMS-56] Pass the correct context in dec_field' #32
Conversation
There was a problem hiding this comment.
LGTM, but I feels that you change mechanically changed all ctxt' and ctxt'' to ctxt that could gut our legs later. I believe there was some idea behind having 2 instances of ctxt: one for declarations and another postponed by lambda continuation for a declaration body.
Please explain all lines that changing what actually code is doing. Because now I cannot where is bug fixes and where is refactoring. Or just leave only necessary changes for a bugfix.
| @@ -336,7 +336,7 @@ and dec_field' ctxt d = | |||
| match d.M.dec.it with | |||
| (* type declarations*) | |||
| | M.(TypD (typ_id, typ_binds, {note = T.Variant cons;_})) -> | |||
| ctxt, None, None, fun _ -> | |||
There was a problem hiding this comment.
I agree that shadowing is better, but here you have changed semantic.
Before we was using ctxt from dec_field' ctxt d which is environment of declarations while ctxt from func ctxt is environment of initialization.
There are different. For example the first former one saw prefix of declarations, while the latter all of them.
If you change semantic, please explain it in each case.
There was a problem hiding this comment.
In the types cases ctxt is passed to the tr_typ. tr_typ requires it because it updates the tuple arities for the subsequent prelude generation.
I guess that when the ctxt was introduced in tr_typ one forgot to add ctxt to the lambda argument.
There was a problem hiding this comment.
It may shoot later. Right now tr_typ refers to the ref field, so, the value inside is the same.
There was a problem hiding this comment.
I think you are not right. I would say that type definition should interpreted under context of previous definitions, so postponed context was omitted on purpose.
We can ask @int-index who was adding ctxt to tr_typ
There was a problem hiding this comment.
Okay I think it's not really important
| let method_args' = List.map (fun (id, t) -> id, tr_typ ctxt' t) method_args in | ||
| let ctxt'' = { ctxt' | ||
| let method_args' = List.map (fun (id, t) -> id, tr_typ ctxt t) method_args in | ||
| let ctxt = { ctxt | ||
| with self = Some self_id.it; | ||
| ids = List.fold_left (fun env (id, t) -> Env.add id.it (Local, t) env) ctxt.ids method_args } |
There was a problem hiding this comment.
here semantic also have been changed.
Before we used global ctxt that comes from argument of dec_field' while and now you are using ctxt from lambda.
Are you sure that it is legit assuption?
There was a problem hiding this comment.
Are you sure that it is legit assuption?
Yeah, actually it was the buggy place. AFAIU the context in the lambda is the one with all top-level definitions. As I see, Motoko doesn't support duplicate top-level names and all top-levels could be accessed in every method
There was a problem hiding this comment.
Actually I've understood what is happened:
Problem is that monorphisation is changing order of declarations:
xarray: [var Nat]
func copy_xarray;
func reverseArray<T>;
becomes
func reverseArray$Nat; // here there is no xarray
xarray: [var Nat]
func copy_xarray;
In the going "top-level"/"declaration" context there is no "xarray" if function is polymorphic because it goes later while in the "body" context (after handling all declarations) it exists.
You are saying that using both contexts in translation of function body was a mistake. Probably very old one. Okay I agree.
I guess main idea behind this code was is to have 1-phase translation handling declarations and definitions at ones.
| let method_args' = List.map (fun (id, t) -> id, tr_typ ctxt' t) method_args in | ||
| let ctxt'' = { ctxt' | ||
| let method_args' = List.map (fun (id, t) -> id, tr_typ ctxt t) method_args in | ||
| let ctxt = { ctxt | ||
| with self = Some self_id.it; | ||
| ids = List.fold_left (fun env (id, t) -> Env.add id.it (Local, t) env) ctxt.ids method_args } |
There was a problem hiding this comment.
The answer is the same as above
|
And also do rebase instead of merge. It's not so important, but maybe for PR in a 1-2 commits it makes more sense and make less work for reviewing them later by Dfinity |
In
dec_field'in some places, we're passing the wrong context (e.g.ctxtinstead ofctxt'). In this PRctxtvariables with ticks got stripped, let's use shadowing to get rid of this inconsistency.