fix: record update should type-check like record creation #5625
Conversation
src/mo_frontend/typing.ml
Outdated
|
|
||
| let find_field (ef : exp_field) (fts : T.field list) = | ||
| let id = ef.it.id.it in | ||
| List.find_opt T.(fun ft -> ft.lab = id && not (is_typ ft.typ)) fts |
There was a problem hiding this comment.
exp_field is always value-level, yes? So we need to skip the type <id> = <typ> T.fields.
normalize not necessary, right?
There was a problem hiding this comment.
Normalize should not be necessary to distinguish a type field from a value field, no. Ditto for T.mut
There was a problem hiding this comment.
There should be a Type.find_val_field_opt or similar that does this for I think.
There was a problem hiding this comment.
Thanks! lookup_val_field_opt is there, but it returns the type instead of the full field, but I've moved the find_val_field_opt next to that
src/mo_frontend/typing.ml
Outdated
| let missing_field_labs = | ||
| List.filter (fun ft -> not (List.exists (fun ft' -> ft.T.lab = ft'.T.lab) fts')) fts | ||
| |> List.map (fun ft -> Printf.sprintf "'%s'" ft.T.lab) | ||
| in | ||
| begin match missing_field_labs with | ||
| | [] -> check_inferred env0 env t t' exp | ||
| | fts -> | ||
| (* Future work: Replace this error with a general subtyping error once better explanations are available. *) | ||
| let s = if List.length fts = 1 then "" else "s" in | ||
| local_error env exp.at "M0151" "missing field%s %s from expected type%a" s (String.concat ", " fts) display_typ_expand t; | ||
| t' | ||
| end |
There was a problem hiding this comment.
All of this should be redundant (replaced with just check_inferred env0 env t t' exp) once we have time to complete #5409
src/mo_frontend/typing.ml
Outdated
| | _ -> [] | ||
| in | ||
| let missing_field_labs = | ||
| List.filter (fun ft -> not (List.exists (fun ft' -> ft.T.lab = ft'.T.lab) fts')) fts |
There was a problem hiding this comment.
I wonder if this should be distinguishing type fields from value fields. They can have the same name. Maybe this was a lurking bug?
There was a problem hiding this comment.
Yes, another limitation of this extra check is that it is not checking whether the type matches.
Anyway, if this check does not catch the error, the check_inferred will. This is just an extra check.
But yeah I'll change to only val fields
| List.iter2 (check_exp env) ts exps; | ||
| t | ||
| | ObjE ([], exp_fields) as e, T.Obj(T.Object, fts) -> (* TODO: infer bases? Default does a decent job. *) | ||
| check_ids env "object" "field" |
There was a problem hiding this comment.
Has this check (check_ids) been removed? I think it checks the field names are distinct and needs to be done somewhere.
There was a problem hiding this comment.
It is still there, in the infer_check_bases_fields
2 participants
Improve type inference for record update syntax. Example
fixes #5628