Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0007137OCaml~DO NOT USE (was: OCaml general)public2016-02-05 17:202016-12-09 12:39
Reporteraalekseyev 
Assigned To 
PrioritynormalSeverityminorReproducibilityalways
StatusclosedResolutionduplicate 
PlatformOSOS Version
Product Version 
Target Version4.03.1+devFixed in Version 
Summary0007137: Make -open support module paths
DescriptionCurrently -open expects a module name, not a module path. This seems overly restrictive. Can we start supporting module paths?
Steps To Reproduceecho 'module B = struct let x = 8 end' > a.ml
echo 'let y = x' > c.ml
ocamlopt.opt -c a.ml
ocamlopt.opt -open A.B -c c.ml
Additional InformationOne might want to use a workaround [-open A -open B], but that additionally brings in scope other definitions from [A], which is not desirable.
TagsNo tags attached.
Attached Files

- Relationships

-  Notes
(0015319)
gasche (developer)
2016-02-05 17:33

That seems like a reasonable request. Would you provide a patch implementing this feature?

Besides, the failure mode when passing a non-atomic path today is not very good usability-wise:

$ ocamlc -open Scanf.Scanning -i test.ml
File "command line", line 1:
Error: Unbound module Scanf.Scanning

$ ocaml -open Scanf.Scanning
        OCaml version 4.02.3

Fatal error: exception Typetexp.Error(_, _, _)
(0015320)
aalekseyev (reporter)
2016-02-05 17:39

I don't have a patch right now, but I may provide one eventually.
(0015377)
aalekseyev (reporter)
2016-02-19 15:39

This is my patch. It seems to work on simple examples and the tests in testsuite/tests/tool-ocamldep-modalias pass, but I'm not sure if this is the only place in the code this command line flag is parsed.

diff --git a/driver/compmisc.ml b/driver/compmisc.ml
index 27efafd..765f6ce 100644
--- a/driver/compmisc.ml
+++ b/driver/compmisc.ml
@@ -45,7 +45,7 @@ let init_path ?(dir="") native =
 let open_implicit_module m env =
   let open Asttypes in
   let lid = {loc = Location.in_file "command line";
- txt = Longident.Lident m } in
+ txt = Longident.parse m } in
   snd (Typemod.type_open_ Override env lid.loc lid)
 
 let initial_env () =
(0015378)
gasche (developer)
2016-02-19 16:18

The patch goes in the right direction, but it needs a bit more work:

- Error handling. You use Longident.parse, which may fail, so we need some sort of error handling. Unfortunately, there is no error handling support in Compmisc current, so you would have to add the machinery. Have a look at driver/pparse.ml for inspiration: you should to define a new `Error` exception in compmisc, with an `error` type with just one constructor (invalid open path?), a `report_error` function, and call `Location.register_error_of_exn`.

- Changelog: you should add a mention of your work in the `Changes` file

- testing: I would need at least one case of non-trivial path passed to -open in the testsuite. You can reuse the makefile of testsuite/tests/tool-ocamldep-modalias, but it should probably go in its own directory because it's another kind of test.

- patch attribution: if I eventually want to merge your patch in, I need a full author name and email for the author attribution. If you are using git, you could just properly configure your author settings and then use `git format-patch` to create the patch, or you could submit a pull request against https://github.com/ocaml/ocaml/ [^] directly.
(0015379)
aalekseyev (reporter)
2016-02-19 16:30

Thanks, I'll do this.
(0015419)
aalekseyev (reporter)
2016-03-02 17:28

I did most of these things, except for error handling. For better or worse, Longident.parse can not fail so it doesn't seem necessary.

I tried to change the type of open_modules command like flag to Longident.t to make parsing closer to where it should be, but that introduces a dependency cycle util -> parsing -> util that needs Makefile changes so I reverted that change.


From 0ccbff9992c907f3296f187806dc1cb04eabc3ab Mon Sep 17 00:00:00 2001
From: Arseniy Alekseyev <aalekseyev@janestreet.com>
Date: Wed, 2 Mar 2016 16:20:33 +0000
Subject: [PATCH] Make "-open" command line flag accept a module path

---
 Changes | 7 +++++++
 driver/compmisc.ml | 2 +-
 testsuite/tests/tool-ocamlc-open/A.ml | 3 +++
 testsuite/tests/tool-ocamlc-open/B.ml | 1 +
 testsuite/tests/tool-ocamlc-open/Makefile | 16 ++++++++++++++++
 tools/ocamldep.ml | 2 +-
 6 files changed, 29 insertions(+), 2 deletions(-)
 create mode 100644 testsuite/tests/tool-ocamlc-open/A.ml
 create mode 100644 testsuite/tests/tool-ocamlc-open/B.ml
 create mode 100644 testsuite/tests/tool-ocamlc-open/Makefile

diff --git a/Changes b/Changes
index bb4139e..cd6b3ca 100644
--- a/Changes
+++ b/Changes
@@ -1,3 +1,10 @@
+Future version of OCaml:
+-------------
+
+Compilers:
+- Make "-open" command line flag accept a module path (not a module name)
+ (Arseniy Alekseyev and Leo White)
+
 OCaml 4.03.0:
 -------------
 
diff --git a/driver/compmisc.ml b/driver/compmisc.ml
index 27efafd..765f6ce 100644
--- a/driver/compmisc.ml
+++ b/driver/compmisc.ml
@@ -45,7 +45,7 @@ let init_path ?(dir="") native =
 let open_implicit_module m env =
   let open Asttypes in
   let lid = {loc = Location.in_file "command line";
- txt = Longident.Lident m } in
+ txt = Longident.parse m } in
   snd (Typemod.type_open_ Override env lid.loc lid)
 
 let initial_env () =
diff --git a/testsuite/tests/tool-ocamlc-open/A.ml b/testsuite/tests/tool-ocamlc-open/A.ml
new file mode 100644
index 0000000..4ae15f1
--- /dev/null
+++ b/testsuite/tests/tool-ocamlc-open/A.ml
@@ -0,0 +1,3 @@
+module M = struct
+ let f x = x +1
+end
diff --git a/testsuite/tests/tool-ocamlc-open/B.ml b/testsuite/tests/tool-ocamlc-open/B.ml
new file mode 100644
index 0000000..6c78157
--- /dev/null
+++ b/testsuite/tests/tool-ocamlc-open/B.ml
@@ -0,0 +1 @@
+let g = f
diff --git a/testsuite/tests/tool-ocamlc-open/Makefile b/testsuite/tests/tool-ocamlc-open/Makefile
new file mode 100644
index 0000000..4167416
--- /dev/null
+++ b/testsuite/tests/tool-ocamlc-open/Makefile
@@ -0,0 +1,16 @@
+SOURCES = A.ml B.ml
+OBJECTS = $(SOURCES:%.ml=Lib%.cmo)
+NOBJECTS = $(OBJECTS:%.cmo=%.cmx)
+
+byte: b.cmo
+clean:
+ rm b.cmo a.cmo
+
+a.cmo: A.ml
+ $(OCAMLC) -c -o $@ $<
+
+b.cmo: B.ml
+ $(OCAMLC) -c -open A.M -o $@ $<
+
+BASEDIR=../..
+include $(BASEDIR)/makefiles/Makefile.common
diff --git a/tools/ocamldep.ml b/tools/ocamldep.ml
index 6e102e9..f3675dc 100644
--- a/tools/ocamldep.ml
+++ b/tools/ocamldep.ml
@@ -292,7 +292,7 @@ let read_parse_and_extract parse_function extract_function def magic
       let bound_vars =
         List.fold_left
           (fun bv modname ->
- Depend.open_module bv (Longident.Lident modname))
+ Depend.open_module bv (Longident.parse modname))
           !module_map !Clflags.open_modules
       in
       let r = extract_function bound_vars ast in
--
1.8.3.1
(0016936)
shinwell (developer)
2016-12-09 08:48

@aalekseyev Please open a Github pull request with your patch so we can close this issue.
(0016942)
aalekseyev (reporter)
2016-12-09 12:26

This is now PR#960

- Issue History
Date Modified Username Field Change
2016-02-05 17:20 aalekseyev New Issue
2016-02-05 17:33 gasche Note Added: 0015319
2016-02-05 17:39 aalekseyev Note Added: 0015320
2016-02-08 12:20 doligez Status new => acknowledged
2016-02-08 12:20 doligez Target Version => 4.03.1+dev
2016-02-19 15:39 aalekseyev Note Added: 0015377
2016-02-19 16:18 gasche Note Added: 0015378
2016-02-19 16:30 aalekseyev Note Added: 0015379
2016-03-02 17:28 aalekseyev Note Added: 0015419
2016-12-07 16:37 doligez Category OCaml tools (ocaml{lex,yacc,dep,browser,debug}) => OCaml tools (ocaml{lex,yacc,dep,debug})
2016-12-09 08:47 shinwell Category OCaml tools (ocaml{lex,yacc,dep,debug}) => OCaml general
2016-12-09 08:48 shinwell Note Added: 0016936
2016-12-09 12:26 aalekseyev Note Added: 0016942
2016-12-09 12:39 shinwell Status acknowledged => closed
2016-12-09 12:39 shinwell Resolution open => duplicate
2017-02-23 16:36 doligez Category OCaml general => -OCaml general
2017-03-03 17:55 doligez Category -OCaml general => -(deprecated) general
2017-03-03 18:01 doligez Category -(deprecated) general => ~deprecated (was: OCaml general)
2017-03-06 17:04 doligez Category ~deprecated (was: OCaml general) => ~DO NOT USE (was: OCaml general)


Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker