Refactor long tuples to records in typeclass.ml

[ulugbekna]

Addressing issue #7927.

I refactored only four functions that take the same long tuple. (Short commits for easier reviewing)
Those four functions are extract_type_decls, merge_type_decls, final_env, and check_coercions.
The long tuple was replaced by a record type that I created 'a class_res. The type's name and its fields' names should probably be changed.

Should that type also be revealed in the interface file? Afaik, 'a class_res is only used by those four functions internal to the typeclass.ml.

[ulugbekna]

All typing-objects tests passed. However, typing-objects-bugs tests were not in the log for some unknown-to-me reason. The tests failed during lib-bigarray-2 testing, but that shouldn't be in relation to my work.

[ulugbekna]

Next I would want to work on tuple (cl, id, ty_id, obj_id, obj_params, obj_ty, cl_id, cl_params, cl_ty, constr_type, dummy_class) used in several places in typeclass.ml, e.g. class_infos function.

[dra27]

This looks like a nice start, thanks! Two related thoughts:

• Try to maximize the use of field punning - i.e. be allergic to writing { foo = bar } if a rename can give { foo } instead
• I would think for these new types (especially ones internal to a module) that we can drop the clsres_ prefix on the field names and simply require a type annotation at the site. i.e. I think that let foo ({id; id_loc; clty} : 'a class_res) = … is nicer than let foo {clsres_id; clsres_id_loc; clsres_clty} = … and definitely nicer than let foo {clsres_id = id; clsres_id_loc = id_loc; clsres_clty = clty} = …. I can't remember if this is something which used not to be possible or whether this kind of a prefixing is just our lpdwHungarian notation hangover from pre-Merlin days!

[gasche]

I think it's okay to have a small scope for a first PR on this topic; settling on record field names for this record and code style for those functions. We can have further PRs that deal with other functions later, even if that means changing the present record to break it in smaller pieces.

I agree with @dra27's suggestions (drop the prefix and use punning as much as possible).

[gasche]

(I'm happy to let @dra27 continue to drive the review for this PR, approving when satisfied with the state.)

[ulugbekna]

Sorry it took me so long; had a midterms week. Let me know what you think about the fix.

Btw, I copied the pre-commit hook to my .git and configured my editor to automatically remove trailing whitespace. Thanks, @dra27 :-)

[dra27]

This LGTM - it's an internal refactoring, so up to you if you'd like to push a Changes entry.

[ulugbekna]

That's a little internal refactoring. I'd say it would rather clutter Changes than add valuable info, right?

[gasche]

We have an Internal/compiler-libs change section in the changelog for internal changes. Having entries for new contributors is nice, so please add a Changes entry there. (If you were to do further refactoring PRs on this file, you could just add them to the same entry instead of having more and more entries.)

[ulugbekna]

We don't need to put a label, e.g. GPR, before the issue number anymore, right? So, I have
- #7927: Replace long tuples into records in typeclass.ml (Ulugbek Abdullaev, review by David Allsopp and Gabriel Scherer)?

[Octachron]

Indeed, however you should also add the pr number:

- #7927, #8527: Replace long tuples into records in typeclass.ml
  (Ulugbek Abdullaev, review by David Allsopp and Gabriel Scherer)

[gasche]

It is an omission on our part to not have merged this PR earlier. I will rebase to fix the conflict, push and merge when the CI passes.