Skip to content

Commit a053b62

Browse files
authored
Speed up record inclusion check. (#6289)
* Speed up record inclusion check. Speed up record inclusion check. Fixes #6284 Record inclusion check (between implementation and interface) is quadratic. Example: ```res module M : { type t<'a, 'b, 'c> = {x:list<('a, 'b)>, y:int, z:int} } = { type t<'a, 'b, 'c> = {x:list<('a, 'c)>, y:int, z:int} } ``` The algorithm tries to instantiate type parameters. It only reports an error if there is an inconsistency. This requires solving type equations involving many types at once. To improve error message, the first problematic field is reported. So the type equations are checked again and again with size 1, 2, ...n where n is the number of fields. (Plus the type parameters). This is quadratic and is problematic for types of ~1K elements. This PR provides a fast path which just checks if there is an error, without blaming a specific field. The fast path is linear. Only if an error is detected, the quadratic path is take to blame precisely which field is involved. * Add text for record inclusion. So that there's some minimal sanity check that record type inclusion works as expected on a nontrivial case. * Update CHANGELOG.md * Refactor into a single function.
1 parent 3ad5cb1 commit a053b62

File tree

4 files changed

+76
-36
lines changed

4 files changed

+76
-36
lines changed

CHANGELOG.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,8 @@
2424
#### :bug: Bug Fix
2525

2626
- Fix issue where uncurried type internals leak in type error. https://github.com/rescript-lang/rescript-compiler/pull/6264
27-
- Improve error messages for untagged variant definitions https://github.com/rescript-lang/rescript-compiler/pull/6290
28-
27+
- Improve error messages for untagged variant definition. https://github.com/rescript-lang/rescript-compiler/pull/6290
28+
- Fix type checking performance issue for large records. https://github.com/rescript-lang/rescript-compiler/pull/6289
2929

3030
# 11.0.0-beta.1
3131

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
2+
We've found a bug for you!
3+
/.../fixtures/RecordInclusion.res:3:5-5:1
4+
5+
1 │ module M : {
6+
2 │ type t<'a, 'b, 'c> = {x:int, y:list<('a, 'b)>, z:int}
7+
3 │ } = {
8+
4 │  type t<'a, 'b, 'c> = {x:int, y:list<('a, 'c)>, z:int}
9+
5 │ }
10+
6 │
11+
12+
Signature mismatch:
13+
...
14+
Type declarations do not match:
15+
type t<'a, 'b, 'c> = {x: int, y: list<('a, 'c)>, z: int}
16+
is not included in
17+
type t<'a, 'b, 'c> = {x: int, y: list<('a, 'b)>, z: int}
18+
/.../fixtures/RecordInclusion.res:2:3-55:
19+
Expected declaration
20+
/.../fixtures/RecordInclusion.res:4:3-55:
21+
Actual declaration
22+
The types for field y are not equal.
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
module M : {
2+
type t<'a, 'b, 'c> = {x:int, y:list<('a, 'b)>, z:int}
3+
} = {
4+
type t<'a, 'b, 'c> = {x:int, y:list<('a, 'c)>, z:int}
5+
}

jscomp/ml/includecore.ml

Lines changed: 47 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -236,39 +236,53 @@ and compare_variants ~loc env params1 params2 n
236236
else compare_variants ~loc env params1 params2 (n+1) rem1 rem2
237237
end
238238

239-
240-
and compare_records ~loc env params1 params2 n
241-
(labels1 : Types.label_declaration list)
242-
(labels2 : Types.label_declaration list) =
243-
match labels1, labels2 with
244-
[], [] -> []
245-
| [], l::_ -> [Field_missing (true, l.Types.ld_id)]
246-
| l::_, [] -> [Field_missing (false, l.Types.ld_id)]
247-
| ld1::rem1, ld2::rem2 ->
248-
if Ident.name ld1.ld_id <> Ident.name ld2.ld_id
249-
then [Field_names (n, ld1.ld_id.name, ld2.ld_id.name)]
250-
else if ld1.ld_mutable <> ld2.ld_mutable then [Field_mutable ld1.ld_id] else begin
251-
Builtin_attributes.check_deprecated_mutable_inclusion
252-
~def:ld1.ld_loc
253-
~use:ld2.ld_loc
254-
loc
255-
ld1.ld_attributes ld2.ld_attributes
256-
(Ident.name ld1.ld_id);
257-
let field_mismatch = !Builtin_attributes.check_bs_attributes_inclusion
258-
ld1.ld_attributes ld2.ld_attributes
259-
(Ident.name ld1.ld_id) in
260-
match field_mismatch with
261-
| Some (a,b) -> [Field_names (n,a,b)]
262-
| None ->
263-
if Ctype.equal env true (ld1.ld_type::params1)(ld2.ld_type::params2)
264-
then (* add arguments to the parameters, cf. PR#7378 *)
265-
compare_records ~loc env
266-
(ld1.ld_type::params1) (ld2.ld_type::params2)
267-
(n+1)
268-
rem1 rem2
239+
and compare_records ~loc env params1_ params2_ n_
240+
(labels1_ : Types.label_declaration list)
241+
(labels2_ : Types.label_declaration list) =
242+
(* First try a fast path that checks if all the fields at once are consistent.
243+
When that fails, try a slow path that blames the first inconsistent field *)
244+
let rec aux ~fast params1 params2 n labels1 labels2 =
245+
match labels1, labels2 with
246+
[], [] ->
247+
if fast then
248+
if Ctype.equal env true params1 params2 then
249+
[]
250+
else
251+
aux ~fast:false params1_ params2_ n_ labels1_ labels2_
269252
else
270-
[Field_type ld1.ld_id]
271-
end
253+
[]
254+
| [], l::_ -> [Field_missing (true, l.Types.ld_id)]
255+
| l::_, [] -> [Field_missing (false, l.Types.ld_id)]
256+
| ld1::rem1, ld2::rem2 ->
257+
if Ident.name ld1.ld_id <> Ident.name ld2.ld_id
258+
then [Field_names (n, ld1.ld_id.name, ld2.ld_id.name)]
259+
else if ld1.ld_mutable <> ld2.ld_mutable then [Field_mutable ld1.ld_id] else begin
260+
Builtin_attributes.check_deprecated_mutable_inclusion
261+
~def:ld1.ld_loc
262+
~use:ld2.ld_loc
263+
loc
264+
ld1.ld_attributes ld2.ld_attributes
265+
(Ident.name ld1.ld_id);
266+
let field_mismatch = !Builtin_attributes.check_bs_attributes_inclusion
267+
ld1.ld_attributes ld2.ld_attributes
268+
(Ident.name ld1.ld_id) in
269+
match field_mismatch with
270+
| Some (a,b) -> [Field_names (n,a,b)]
271+
| None ->
272+
let current_field_consistent =
273+
if fast then true
274+
else Ctype.equal env true (ld1.ld_type::params1)(ld2.ld_type::params2) in
275+
if current_field_consistent
276+
then (* add arguments to the parameters, cf. PR#7378 *)
277+
aux ~fast
278+
(ld1.ld_type::params1) (ld2.ld_type::params2)
279+
(n+1)
280+
rem1 rem2
281+
else
282+
[Field_type ld1.ld_id]
283+
end in
284+
aux ~fast:true params1_ params2_ n_ labels1_ labels2_
285+
272286

273287
let type_declarations ?(equality = false) ~loc env name decl1 id decl2 =
274288
Builtin_attributes.check_deprecated_inclusion
@@ -324,8 +338,7 @@ let type_declarations ?(equality = false) ~loc env name decl1 id decl2 =
324338
if equality then mark cstrs2 Env.Positive (Ident.name id) decl2;
325339
compare_variants ~loc env decl1.type_params decl2.type_params 1 cstrs1 cstrs2
326340
| (Type_record(labels1,rep1), Type_record(labels2,rep2)) ->
327-
let err = compare_records ~loc env decl1.type_params decl2.type_params
328-
1 labels1 labels2 in
341+
let err = compare_records ~loc env decl1.type_params decl2.type_params 1 labels1 labels2 in
329342
if err <> [] || rep1 = rep2 then err else
330343
[Record_representation (rep1, rep2)]
331344
| (Type_open, Type_open) -> []

0 commit comments

Comments
 (0)