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

the typer fails reading unnecessary cmis with -no-alias-deps and -w -49 #6998

Closed
vicuna opened this issue Sep 24, 2015 · 5 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link

commented Sep 24, 2015

Original bug ID: 6998
Reporter: @sliquister
Status: closed (set by @xavierleroy on 2017-09-24T15:33:24Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.02.2
Fixed in version: 4.03.0+dev / +beta1
Category: typing
Related to: #6843 #7325
Monitored by: @diml

Bug description

As you can see in the script below, when aliasing a compilation unit, the compiler tries to read the cmi for that compilation unit.

If the cmi doesn't exit, a 'try with Not_found' in Env.lookup_module makes the typer succeed. If the cmi can be read, but it is not consistency with the other cmis, the typer succeeds because the consistency checked is skipped. There are other potentially errors caught in Env.read_pers_struct that are bypassed. But if the existing cmi on disk is for a different version of ocaml, or is partially written out by a concurrent call to the compiler, or generally speaking can't be read (no read permission for instance), then the typing fails.

I think this is wrong and no error should ever be given: there is no dependency between a.cmi and b.cmi in the example, so the behavior of the typer when building a.cmi should not depend on b.cmi. In the current state of the typer, I think we can't build a tree at the current version of ocaml and then at the next version of ocaml without cleaning the tree (assuming the cmi format changed) because of this.

I suppose two fixes are possible:

  • catch all the exceptions from find_pers_struct in Env.lookup_module instead of just Not_found
  • don't even call find_pers_struct when warning 49 is off, still in the same function. add_import still needs to be called though.

Steps to reproduce

#!/bin/bash

cd /tmp
rm -f .cm
echo 'a.ml builds if b.cmi is not present'
echo 'module B = B' > a.ml
ocamlc.opt -w -49 -no-alias-deps -c a.ml
echo 'but if some old garbage b.cmi is present'
touch b.cmi
ocamlc.opt -w -49 -no-alias-deps -c a.ml

outputs:

a.ml builds if b.cmi is not present

but if some old garbage b.cmi is present

File "a.ml", line 1:

Error: Corrupted compiled interface

b.cmi

File attachments

@vicuna

This comment has been minimized.

Copy link
Author

commented Oct 2, 2015

Comment author: @sliquister

So I confirm that cleaning between compiler changes is necessary because of this bug. I tried the attached patch, which solves the problem (but other than that, I haven't really tested it). I went with my second proposal, I think there are only the following slight behavior changes:

  • if warning 49 is on, with the patch, we check consistency of the cmi. It's just strange that right now having an inconsistent cmi is ok, but if -rectypes is not consistent then it's an error.
  • if warning 49 is off, now we don't even try to load cmis, which fixes my problem and probably makes the compiler a bit faster. Env.persistent_structures doesn't contain the names of these compilation unit anymore, but it seems that it's only used for short-paths and suggestions on typos, so it sounds fine. Especially given it's not exactly a good idea to have warning 49 turned off for hand written code.
@vicuna

This comment has been minimized.

Copy link
Author

commented Oct 21, 2015

Comment author: @lpw25

There's a patch for this at: #261

@vicuna

This comment has been minimized.

Copy link
Author

commented Oct 23, 2015

Comment author: @garrigue

Leo, do you mean that we should merge that patch?
Since you introduced warning 49, I suppose you're in charge of that.

@vicuna

This comment has been minimized.

Copy link
Author

commented Oct 23, 2015

Comment author: @lpw25

I'm waiting to see if anyone has any comments or review before merging. I'll merge once its clear no one has anything to say about it.

@vicuna

This comment has been minimized.

Copy link
Author

commented Nov 15, 2015

Comment author: @xavierleroy

Fix merged in trunk early Nov 2015.

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