links-lang / links

Links: Linking Theory to Practice for the Web
http://www.links-lang.org
Other
333 stars 43 forks source link

Compile with OCaml 5.0.0 #1165

Closed dhil closed 1 year ago

dhil commented 1 year ago

This patch makes our code base compile with OCaml 5.0.0, whilst maintaining compatibility with OCaml 4.08+.

This patch also changes the CI to use OCaml 4.14.1 and OCaml 5.0.0.

dhil commented 1 year ago

OCaml 4.14.1 triggers a failure in the Lens testsuite

==============================================================================
Error: all:0:lenses:2:lens_fd_helpers:4:tree_form:2:key_overlap.

File "/home/runner/work/links/links/_build/oUnit-all-fv-az404-48#00.log", line 90, characters 1-1:
Error: all:0:lenses:2:lens_fd_helpers:4:tree_form:2:key_overlap (in the log).

Raised at OUnitAssert.assert_failure in file "src/lib/ounit2/advanced/oUnitAssert.ml", line 45, characters 2-27
Called from OUnitRunner.run_one_test.(fun) in file "src/lib/ounit2/advanced/oUnitRunner.ml", line 83, characters 13-26

expected: (Fun_dep.Check_error.FunDepNotTreeForm {})
but got: (Fun_dep.Check_error.FunDepNotTreeForm {B; C; })
------------------------------------------------------------------------------
Ran: 64 tests in: 1.16 seconds.
FAILED: Cases: 64 Tried: 64 Errors: 0 Failures: 1 Skip:  5 Todo: 0 Timeouts: 0.
jamescheney commented 1 year ago

failure in the Lens testsuite

Frlom what I can tell, the function is_disjoint (which is the only place where the FunDepNotTreeForm error can be generated) traverses the candidate tree form of the functional dependencies in an unspecified order, checking for each node that the column names associated with the node are valid and disjoint from those encountered so far, and builds up a set of the columns correctly processed until the whole tree/forest is checked. If not, then the set of columns processed so far is returned with the error, for no reason that is apparent to me. This set is not used anywhere else except in checking the result of this unit test, and depending on what order the functional dependencies are traversed, the returned set might differ.

The actual result of the test is correct, i.e. the test correctly finds this set of FDs is not in tree form, it's just that the extra "error description" is different possibly due to some change in an underlying library function used for the sets. I think a sensible solution would be to just remove the set argument of FunDepNotTreeForm (changing it to match the current result would temporarily work too, but be fragile in the face of future library changes or nondeterminism.)

@rudihorn do you have any other thoughts?

rudihorn commented 1 year ago

Ideally I would probably have suggested trying to either see if it can be turned into a deterministic test, but I guess the current approach will do just fine. But speaking of this I still had quite a few unpushed changes which I have just added as a PR.