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.

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.

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.

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.

Comment author: @sliquister

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

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)

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.

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.

Comment author: @garrigue

Tentative fix using sliquister's idea at #774

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

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.

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.

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).

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?)

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

Looks like it was fixed.