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

Warning for redundant open M and let open M in #5357

Closed
vicuna opened this Issue Sep 16, 2011 · 7 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link
Collaborator

vicuna commented Sep 16, 2011

Original bug ID: 5357
Reporter: furuse
Assigned to: @alainfrisch
Status: closed (set by @xavierleroy on 2013-08-31T10:49:10Z)
Resolution: fixed
Priority: normal
Severity: feature
Version: 3.12.1
Fixed in version: 3.13.0+dev
Category: ~DO NOT USE (was: OCaml general)
Related to: #5438
Monitored by: meyer @protz mehdi "Pascal Cuoq" dario @ygrek @glondu "Julien Signoles" @hcarty @Chris00 nogin @garrigue

Bug description

GHC has a warning for never used imports; such imports are just redundant and cause unexpected name space contamination. The warning is useful to keep up your import list minimal as possible.

OCaml's open has the same issue of the name space contamination, and unnecessary opens should be warned, too.

Additional information

And I have added a new warning for it.

You can obtain the latest diff for OCaml 3.12.1 from my repo at https://bitbucket.org/camlspotter/mutated_ocaml , redundant_open_warning branch.

With this patch, I have found nearly 150 redundant opens in OCaml source code!

A global hashtbl and side effects are used to minimize the amount of modifications for easy patching. If we are going to implement this feature in a future OCaml officially, we could make it cleaner.

File attachments

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 21, 2011

Comment author: @protz

That sounds like a pretty cool warning to have, and the patch seems to be moderately invasive. Jacques, any opinion on this?

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 22, 2011

Comment author: @garrigue

After reading it, it seems to do something useful in a straightforward way.

My only reservation is the "with_open" type constructor.
I would prefer something cleaner, like
type 'a info = Path.t * 'a * Path.t option
but this could actually make things more complicated...
This can easily be fixed afterwards.

Also we should probably remove all unneeded open's from the sources,
so there is no reason to disable the warning.

I'm for merging, but maybe better to ask on caml-devel first.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 22, 2011

Comment author: @alainfrisch

Something I don't understand with the implementation is that "open statements" are identified simply with the opened path. So if we have two opens on the same module, and only one is actually used, the other will not be reported as being unused?

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 22, 2011

Comment author: furuse

Jacques: I was kicked out from caml-devel... :-(

Alain: Yeah, you are right. It should be better if we can detect all the unused open M's once and for all. I did not write that part just for simplicity.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 22, 2011

Comment author: @alainfrisch

I've added a similar warning to the unused_declarations branch:

http://caml.inria.fr/cgi-bin/viewvc.cgi?view=revision&revision=11937

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jan 18, 2012

Comment author: @alainfrisch

See #5438. The new warning 33 detects unused open statements.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented May 3, 2012

Comment author: furuse

Thanks, the new warnings rock!

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.