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

Make -open support module paths #7137

Closed
vicuna opened this issue Feb 5, 2016 · 8 comments
Closed

Make -open support module paths #7137

vicuna opened this issue Feb 5, 2016 · 8 comments

Comments

@vicuna
Copy link

vicuna commented Feb 5, 2016

Original bug ID: 7137
Reporter: aalekseyev
Status: closed (set by @mshinwell on 2016-12-09T11:39:19Z)
Resolution: duplicate
Priority: normal
Severity: minor
Target version: 4.03.1+dev
Category: ~DO NOT USE (was: OCaml general)
Monitored by: @lpw25 @hcarty

Bug description

Currently -open expects a module name, not a module path. This seems overly restrictive. Can we start supporting module paths?

Steps to reproduce

echo '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 information

One might want to use a workaround [-open A -open B], but that additionally brings in scope other definitions from [A], which is not desirable.

@vicuna
Copy link
Author

vicuna commented Feb 5, 2016

Comment author: @gasche

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(_, _, _)

@vicuna
Copy link
Author

vicuna commented Feb 5, 2016

Comment author: aalekseyev

I don't have a patch right now, but I may provide one eventually.

@vicuna
Copy link
Author

vicuna commented Feb 19, 2016

Comment author: aalekseyev

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 () =

@vicuna
Copy link
Author

vicuna commented Feb 19, 2016

Comment author: @gasche

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.

@vicuna
Copy link
Author

vicuna commented Feb 19, 2016

Comment author: aalekseyev

Thanks, I'll do this.

@vicuna
Copy link
Author

vicuna commented Mar 2, 2016

Comment author: aalekseyev

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

@vicuna
Copy link
Author

vicuna commented Dec 9, 2016

Comment author: @mshinwell

@aalekseyev Please open a Github pull request with your patch so we can close this issue.

@vicuna
Copy link
Author

vicuna commented Dec 9, 2016

Comment author: aalekseyev

This is now PR#960

@vicuna vicuna closed this as completed Dec 9, 2016
@vicuna vicuna added this to the 4.03.1 milestone Mar 14, 2019
@vicuna vicuna added the bug label Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant