Turn warning 31 (Module_linked_twice) into a hard error #11635

hhugo · 2022-10-17T10:07:04Z

hhugo · 2022-10-17T10:27:27Z

v-gb

Based on the issues linked, there seems to be consensus for making this change, and I think the code change does what's intended.

I tested the behavior of this change with the script at the bottom of this comment, and it works fine but I observe error messages that are a bit worse:

4.14:

native:
File "_none_", line 1:
Error: Files 2/a.cmx and 1/a.cmx both define a module named A

bytecode:
File "2/a.cmo", line 1:
Error (warning 31 [module-linked-twice]): files 2/a.cmo and 1/a.cmo both define a module named A
File "2/b.cmo", line 1:
Error (warning 31 [module-linked-twice]): files 2/b.cmo and 1/b.cmo both define a module named B

this PR:

native:
File "_none_", line 1:
Error: Files 2/a.cmx and 1/a.cmx
       make inconsistent assumptions over interface A

bytecode:
File "_none_", line 1:
Error: Files 2/a.cmo and 1/a.cmo
       make inconsistent assumptions over interface A

Not the end of the world when the build system ensures uniqueness of compilation units, but I think it's trivial to fix (as I indicated in an inline comment), so may as well.

Other than addressing the review comments, please do the usual rebase, adding a Changes entry (and maybe squash the commits? not sure what's expected).

The test script:

#!/bin/bash
set -e -u -o pipefail
which ocamlopt
which ocamlc
rm -rf /tmp/test
mkdir /tmp/test
cd /tmp/test
mkdir 1 2
echo "type t = int" > 1/a.ml
echo "Printf.printf \"%d\\n\" (1 : A.t)" > 1/b.ml
echo "type t = string" > 2/a.ml
echo "Printf.printf \"%s\\n\" (\"qwerty\" : A.t)" > 2/b.ml
echo "native:"
(cd 1; ocamlopt -c {a,b}.ml)
(cd 2; ocamlopt -c {a,b}.ml)
ocamlopt 1/{a,b}.cmx 2/{a,b}.cmx || true
echo "bytecode:"
(cd 1; ocamlc -c {a,b}.ml)
(cd 2; ocamlc -c {a,b}.ml)
ocamlc 1/{a,b}.cmo 2/{a,b}.cmo

v-gb · 2023-01-04T03:47:05Z

asmcomp/asmlink.ml

@@ -90,7 +87,7 @@ let check_consistency file_name unit crc =
  with Not_found -> ()


Based on my test script, I think we should move the raise (Error(Multiple_definition ...)) above at the top of the function, so the errors are improved.

v-gb · 2023-01-04T03:48:51Z

bytecomp/bytelink.ml

+  | Module_linked_twice (modname, file1, file2) ->
+      fprintf ppf "Files %a and %a both define a module named %s"
+        Location.print_filename file1
+        Location.print_filename file2
+        modname
+
+


I think we should use the same code as in asmlink.ml (more format boxes than here), and in fact let's use the same constructor name as well (Multiple_definition), for easier comparison between bytelink.ml and asmlink.ml.

v-gb · 2023-01-04T03:49:29Z

bytecomp/bytelink.ml

-      (Warnings.Module_linked_twice(cu.cu_name,
-                                    Location.show_filename file_name,
-                                    Location.show_filename source))
+    raise (Error (Module_linked_twice(cu.cu_name, file_name, source)));


Same thing as in asmlink.ml: I think we should move the check to the top of the function, for better error messages.

hhugo · 2023-01-04T11:39:36Z

Thanks for the review @sliquister. I meant to add a new test in the testsuite but my few attempts weren't successful.
Here is the output of your script with this PR

native:
File "_none_", line 1:
Error: Files 2/a.cmx and 1/a.cmx both define a module named A
bytecode:
File "_none_", line 1:
Error: Files 2/a.cmo and 1/a.cmo both define a module named A

Running more tests, I noticed that, when linking a library that contain units also present elsewhere on the command line, we don't fail/warn if theses units don't end up being linked. The behavior seems fine but it means that adding -linkall would suddenly trigger the error.

v-gb

Looks good, modulo the Changes entry.

v-gb · 2023-01-04T13:29:12Z

Changes

@@ -219,6 +219,9 @@ Working version
  modules with mismatching -for-pack
  (Pierre Chambart and Vincent Laviron, review by Mark Shinwell)

+- #11635: Turn warning 31 (Module_linked_twice) into a hard error


Please list the issues as well, not just the PR.

v-gb · 2023-01-04T13:42:33Z

The appveyor failure looks spurious.

Running more tests, I noticed that, when linking a library that contain units also present elsewhere on the command line, we don't fail/warn if theses units don't end up being linked. The behavior seems fine but it means that adding -linkall would suddenly trigger the error.

Yeah, that behavior seems fine, although a stricter behavior would probably also make sense.

gasche · 2023-01-04T14:01:34Z

utils/warnings.ml

-  { number = 31;
-    names = ["module-linked-twice"];
-    description = "A module is linked twice in the same executable.";
-    since = since 4 0 };


I would like to understand well what is going on with the warning from a compatibility perspective:

what happens if a user manually specifies this warning, on the command-line or in an attribute, by number or by name?

what is shown in -warn-help?

My current understanding is as follows:

Warning modifiers are interpreted by mutating dense flag arrays of all warning numbers.

Using a number-based modifier -31 or +31 will modify the flag array and do nothing else (as the warning is never raised anymore). In practice this is silently ignored, which is probably fine.

Using -module-linked-twice will result in a lookup failure because the description entry is removed here, which will eventually result in a confusing ocaml_deprecated_cli alert. This is not good.

The warning will not appear in -warn-help anymore, because the description entry is removed.

I think that we should do this instead: keep the description entry, but extend the description to say that the warning is now ignored and replaced by a hard error.

My understanding is that we would then get the following behavior:

specifying the warning by number or by name is ignored

the warning shows up in -warn-help, with a mention that it is ignored

I think that this new behavior would be reasonable, a better default.

One may think of something even more informative (if users try to disable the warning, letting them know that this does not work anymore through... another warning?), but I think that it would not be worth the implementation and conceptual complexity.

Minor note: the definition of defaults_warn_error further down still mentions +31, it should be removed I think.

I've kept the description entry, updated defaults_warn_error and updated manual and man pages.

I'm not super happy with the updated description.

31 [module-linked-twice] Ignored: now a hard error (since 5.0). A module is linked twice in the same executable.

I've tried to follow what was done for number 25

25 Ignored: now part of warning 8.

I've changed it to

31 [module-linked-twice] A module is linked twice in the same executable. Ignored: now a hard error (since 5.0).

Fine with me, thanks!

5.1 rather than 5.0 ?

@Octachron, fixed thanks.

gasche

i agree with @sliquister that this is a good implementation of the proposed change. I am in favor of merging once the commits have been rebased/fixupped by @hhugo as he prefers, and the CI is green.

Thanks a lot @sliquister for your careful review. I don't think that I would have had the energy to look at this by myself without your contribution.

hhugo · 2023-01-04T15:02:54Z

i agree with @sliquister that this is a good implementation of the proposed change. I am in favor of merging once the commits have been rebased/fixupped by @hhugo as he prefers, and the CI is green.

Thanks a lot @sliquister for your careful review. I don't think that I would have had the energy to look at this by myself without your contribution.

I've squashed the commits and rebased.

v-gb · 2023-01-04T16:27:00Z

Looks like the warning changes are breaking tests/warnings/mnemonics.mll.

nojb · 2023-01-04T19:39:12Z

Looks like the warning changes are breaking tests/warnings/mnemonics.mll.

Indeed. This test checks that every constructor of the sum type has a corresponding textual mnemonic, and vice-versa. This fails because the mnemonic for warning 31 does not have a corresponding constructor.

The simplest workaround is to add a hard-coded list of deprecated warnings to this test (for which there is no associated constructor).

hhugo · 2023-01-04T22:52:38Z

I've pushed a fix for the tests/warnings/mnemonics.mll test following @nojb suggestion

hhugo · 2023-01-04T23:05:39Z

The appveyor failure is due to #11853

nojb · 2023-01-05T04:58:13Z

Merged, thanks!

hhugo marked this pull request as ready for review October 28, 2022 11:51

v-gb suggested changes Jan 4, 2023

View reviewed changes

hhugo force-pushed the hhugo-double-link branch from 188e8b9 to 96b8ebf Compare January 4, 2023 11:28

v-gb approved these changes Jan 4, 2023

View reviewed changes

gasche reviewed Jan 4, 2023

View reviewed changes

gasche approved these changes Jan 4, 2023

View reviewed changes

Turn warning 31 (Module_linked_twice) into a hard error

22568bf

hhugo force-pushed the hhugo-double-link branch from b2d6bb0 to 22568bf Compare January 4, 2023 15:01

fix tests

1fe6a09

v-gb approved these changes Jan 5, 2023

View reviewed changes

nojb merged commit 0ec8679 into ocaml:trunk Jan 5, 2023

hhugo deleted the hhugo-double-link branch January 5, 2023 07:20

jchavarri mentioned this pull request Sep 22, 2023

utop: warning when modules collide with utop deps (will become error in 5.1) ocaml/dune#8735

Open

gretay-js mentioned this pull request Oct 20, 2023

Fix backend conflicts ocaml-flambda/ocaml-jst#174

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn warning 31 (Module_linked_twice) into a hard error #11635

Turn warning 31 (Module_linked_twice) into a hard error #11635

hhugo commented Oct 17, 2022 •

edited

hhugo commented Oct 17, 2022

v-gb left a comment

v-gb Jan 4, 2023

hhugo Jan 4, 2023

v-gb Jan 4, 2023

hhugo Jan 4, 2023

v-gb Jan 4, 2023

hhugo Jan 4, 2023

hhugo commented Jan 4, 2023

v-gb left a comment

v-gb Jan 4, 2023

hhugo Jan 4, 2023

v-gb commented Jan 4, 2023

gasche Jan 4, 2023

hhugo Jan 4, 2023

hhugo Jan 4, 2023

gasche Jan 4, 2023

Octachron Jan 4, 2023

hhugo Jan 4, 2023

gasche left a comment

hhugo commented Jan 4, 2023

v-gb commented Jan 4, 2023

nojb commented Jan 4, 2023

hhugo commented Jan 4, 2023

hhugo commented Jan 4, 2023

nojb commented Jan 5, 2023

		@@ -90,7 +87,7 @@ let check_consistency file_name unit crc =
		with Not_found -> ()

Turn warning 31 (Module_linked_twice) into a hard error #11635

Turn warning 31 (Module_linked_twice) into a hard error #11635

Conversation

hhugo commented Oct 17, 2022 • edited

hhugo commented Oct 17, 2022

v-gb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhugo commented Jan 4, 2023

v-gb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v-gb commented Jan 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gasche left a comment

Choose a reason for hiding this comment

hhugo commented Jan 4, 2023

v-gb commented Jan 4, 2023

nojb commented Jan 4, 2023

hhugo commented Jan 4, 2023

hhugo commented Jan 4, 2023

nojb commented Jan 5, 2023

hhugo commented Oct 17, 2022 •

edited