Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compiler forcing aliases it shouldn't while reporting type errors #7134

Open
vicuna opened this issue Feb 3, 2016 · 13 comments

Comments

@vicuna
Copy link

commented Feb 3, 2016

Original bug ID: 7134
Reporter: @sliquister
Assigned to: @garrigue
Status: assigned (set by @sliquister on 2016-08-23T17:07:39Z)
Resolution: open
Priority: normal
Severity: feature
Version: 4.02.3
Target version: undecided
Category: typing
Related to: #6812 #7751

Bug description

This is a similar problem to #6812, the compiler reads cmis it shouldn't, resulting in inconsistent assumptions. But if the cmi doesn't exist, the compiler doesn't read it and so succeeds (at giving a type error).

Stack trace at the moment when the wrong cmi is read (lib__C.cmi in the example below):
#0 0x000000000050c4b0 in camlEnv__read_pers_struct_1510 ()
#1 0x000000000050d22f in camlEnv__find_module_1610 ()
#2 0x00000000005102a0 in camlEnv__scrape_alias_2090 ()
#3 0x0000000000511ea7 in camlEnv__components_of_module_maker_2201 ()
#4 0x000000000050ba2c in camlEnv__force_1253 ()
#5 0x000000000050fccc in camlEnv__find_all_comps_2002 ()
#6 0x0000000000602df1 in camlList__map_1040 () at list.ml:55
#7 0x0000000000602e08 in camlList__map_1040 () at list.ml:55
#8 0x000000000050ff1a in camlEnv__find_shadowed_2019 ()
#9 0x000000000050ff5d in camlEnv__find_shadowed_types_2029 ()
#10 0x0000000000540742 in camlPrinttyp__is_unambiguous_1550 ()
#11 0x000000000053cdb3 in camlPrinttyp__fun_3062 ()
#12 0x0000000000602f81 in camlList__iter_1061 () at list.ml:73
#13 0x00000000005409c3 in camlPrinttyp__get_best_path_1562 ()
#14 0x0000000000540ada in camlPrinttyp__best_type_path_1568 ()
#15 0x000000000053d248 in camlPrinttyp__pr_typ_1679 ()
#16 0x000000000053c49b in camlPrinttyp__pr_arrow_1697 ()
#17 0x0000000000543379 in camlPrinttyp__tree_of_value_description_1908 ()
#18 0x0000000000558341 in camlIncludemod__fun_1891 ()
#19 0x000000000062e797 in camlFormat__output_acc_1501 () at format.ml:1115
#20 0x000000000062ea5c in camlFormat__output_acc_1501 () at format.ml:1125
#21 0x000000000062e797 in camlFormat__output_acc_1501 () at format.ml:1115
#22 0x000000000062e97f in camlFormat__output_acc_1501 () at format.ml:1128
#23 0x000000000062e797 in camlFormat__output_acc_1501 () at format.ml:1115
#24 0x000000000062b710 in camlFormat__fun_2372 () at format.ml:1175
#25 0x000000000055ad61 in camlIncludemod__include_err_1460 ()
#26 0x000000000062e797 in camlFormat__output_acc_1501 () at format.ml:1115
#27 0x000000000062b710 in camlFormat__fun_2372 () at format.ml:1175
#28 0x00000000004a59c4 in camlMisc__try_finally_1011 ()
#29 0x000000000062e797 in camlFormat__output_acc_1501 () at format.ml:1115
#30 0x000000000062b710 in camlFormat__fun_2372 () at format.ml:1175
#31 0x00000000004ae0dc in camlLocation__error_of_printer_1190 ()
#32 0x000000000055ba39 in camlIncludemod__fun_2018 ()
#33 0x00000000004ac982 in camlLocation__loop_1167 ()
#34 0x00000000004ae2ea in camlLocation__report_exception_rec_1202 ()
#35 0x000000000044b825 in camlOptmain__main_1484 ()
#36 0x000000000044cdf4 in camlOptmain__entry ()
#37 0x0000000000447249 in caml_program ()
#38 0x00000000006497d6 in caml_start_program ()

Steps to reproduce

#!/bin/bash

rm -rf /tmp/inconsistent-assumptions
mkdir -p /tmp/inconsistent-assumptions
cd /tmp/inconsistent-assumptions

cat > a.ml <<EOF
type t
EOF

cat > b.ml <<EOF
module C = struct
type t = { a : A.t }
let f t = t.a
end

include C
EOF

cat > b.mli <<EOF
val f : string
EOF

touch lib__C.cmi # let's pretend we have a version of c.ml
# that was built against a different a.cmi

cat > lib.ml <<EOF
module A = Lib__A
module B = Lib__A
module C = Lib__C
EOF

ocaml=/j/office/app/ocaml/builds/4.02.3+gc-2+j2-cent6_20151110_095047GMT/bin/ocamlopt.opt
$ocaml -nostdlib -nopervasives -no-alias-deps -w +a-49 -short-paths -o lib.cmx -c lib.ml
$ocaml -nostdlib -nopervasives -no-alias-deps -w +a -short-paths -open Lib -o lib__A.cmx -c a.ml
$ocaml -nostdlib -nopervasives -no-alias-deps -w +a -short-paths -open Lib -o lib__B.cmi -c b.mli
$ocaml -nostdlib -nopervasives -no-alias-deps -w +a -short-paths -open Lib -o lib__B.cmx -c b.ml

-nostdlib and -nopervasives are not necessary, they just reduce irrelevant noise

results in:

File "b.ml", line 1:

Error: Corrupted compiled interface

lib__C.cmi

instead of a proper type error

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 25, 2016

Comment author: @garrigue

I'm not sure what to do here.
This is not a bug of the compiler: as always, it returns the first error it found, and having a wrong cmi in your path is indeed an error.
Your point seems to be that since not having a cmi is actually legal, we should not fail in that case, just ignore the broken file. But wouldn't it be even more confusing, if you were actually expecting a cmi for that file?
To me this looks more like a hygiene problem than a compiler feature.

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 25, 2016

Comment author: @garrigue

Oh, wait a minute. Does the problem only occur with -short-paths?
In that case it might be related to the way -short-paths explores its environment before reporting errors. The code was written before the introduction of module aliases, and there may be bad interactions.

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 25, 2016

Comment author: @sliquister

Yeah, I think it's only with short-paths and -no-alias-deps, otherwise modules can't even alias modules they don't depend on.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 29, 2016

Comment author: @sliquister

This bites people occasionally. Instead of finding every place in the typer that might be forcing aliases to compilation units that the input program doesn't use, how about:

  • adding 'val Env.can_load_cmis : bool ref = ref false'
  • using it in Env.find_pers_struct to refuse to load cmis
  • setting it to true around the part of the typer where it does error reporting

This assumes the typer doesn't have legitimate reasons to load compilation units that weren't loaded at the time the type error was found. Not sure if this is true.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 29, 2016

Comment author: @sliquister

Oops, I meant the ref defaults to true and would be set to false for error reporting.

@vicuna

This comment has been minimized.

Copy link
Author

commented Aug 1, 2016

Comment author: @sliquister

Alternatively, I think (haven't tested) that something like #682 would allow to change the way we build to not produce these forward references anymore.
And without forward references, this problem (and at least one other I have reported and was fixed) go away.

The idea would be that instead of creating, for each library, a module aliasing all the modules of the library, and make every module of the library open the aliases module, I would have a preprocessor that would create just the aliases a given module needs.
In the example above, b.ml would contain (after preprocessing):
open struct module A = Lib__A end (* from preprocessor *)
module C = struct
type t = { a : A.t }
let f t = t.a
end
and so the compiler would have no reason to look for lib__C.cmi.

(and this would also require a similar feature in signatures: open sig .. end)

@vicuna

This comment has been minimized.

Copy link
Author

commented Aug 3, 2016

Comment author: @garrigue

#7134#c16151 makes sense to me.
The compiler indeed has no good reason to load cmis when reporting error: if it found an error, it has already loaded all the relevant cmis that are involved in the error.
I'll see if it can be done cleanly.

Your workaround might work, but we don't want to force all users to use such an approach.

@vicuna

This comment has been minimized.

Copy link
Author

commented Aug 3, 2016

Comment author: @garrigue

By the way, Frédéric Bour was supposed to change the way -short-paths works.
He has a cleaner approach, which exploits nicely module aliases.
Still waiting for it.

@vicuna

This comment has been minimized.

Copy link
Author

commented Aug 22, 2016

Comment author: @garrigue

Tentative fix using sliquister's idea at #774

Could you quickly tell me if this solves all your problems?

@vicuna

This comment has been minimized.

Copy link
Author

commented Aug 23, 2016

Comment author: @sliquister

The tip of the feature is broken because the second commit sets the boolean to true instead of false, effectively doing nothing.
But other than that:

  • the test in the description passes
  • the problem that made me ping this PR in July gives a type error rather than inconsistent assumptions (when I backport the patch, I can't use a new compiler version)

I haven't tested the problem that made me create this PR, I'm going to try now if it's not too hard.

@vicuna

This comment has been minimized.

Copy link
Author

commented Aug 23, 2016

Comment author: @sliquister

I reproduced the bug from February, and this one also gives a type error instead of inconsistent assumptions with this change.

I wonder if the fix from #6812 is still necessary, with this.

@vicuna

This comment has been minimized.

Copy link
Author

commented Aug 23, 2016

Comment author: @sliquister

I checked the reproduction from #6812 with 4.02.1 (inconsistent assumptions) and 4.02.1 + this patch (the expected type error).

@vicuna

This comment has been minimized.

Copy link
Author

commented Aug 23, 2016

Comment author: @garrigue

Fixed in 4.04 by GPR #774.

Should add a test case, and check whether the fix of 6812 is really needed now.
(It does something different, but is it semantically useful?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.