Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0005980OCamlOCaml generalpublic2013-04-10 13:052014-02-20 23:10
Reporterdbuenzli 
Assigned Tofrisch 
PrioritynormalSeverityfeatureReproducibilityhave not tried
StatusresolvedResolutionfixed 
PlatformOSOS Version
Product Version4.00.1 
Target VersionFixed in Version4.01.0+dev 
Summary0005980: Maybe a new warning for M.() notation ?
DescriptionWhile generally useful I think that the M.() notation may sometimes lead to subtle bugs when used as an expression.

Suppose I have M with this signature:

module M : sig
  type t
  val id : t
  val ( + ) : t -> t -> t
  ...
end

And I code this function:

let f () =
  let (id : t) = ... in
  let (c : t) = ... in
  M.(id + c)

I would like a warning that tells me that in M.(id + c), id is overriding a local variable. Note that a few cases are already covered by current warnings/errors.

1) If the local id not used elsewhere in f you will have (an initially puzzling) unused variable warning -- however that warning doesn't cover pattern matching variables.

2) If M.id was of another type you'd get an (aswell puzzling) type mismatch error.

I think that failing to have such a warning by default somehow renders all M.(e), where e binds local variables quite brittle and suspicious.

Thanks,

Daniel


   

TagsNo tags attached.
Attached Files

- Relationships

-  Notes
(0009059)
frisch (developer)
2013-04-10 13:13

> I would like a warning that tells me that in M.(id + c), id is overriding a local variable

Do you suggest to treat differently the two syntaxes "M.(e)" and "let open M in e"? I'd rather add a warning for both, and also for structure-level opens. Concretely, I believe the warning should be on the reference to "id", i.e. shadowing a local variable with a local open is fine as long as we don't use it in the scope of the open.

What about a warning arbitrary ambiguous references (referring to an identifier imported in the current scope by some open, but which was already present in the environment before that open)?


> however that warning doesn't cover pattern matching variables.

Did you enable warning 27? (For some reason, it was considered at some point that unused pattern variables are less suspicious than others.)
(0009063)
dbuenzli (reporter)
2013-04-10 14:11

> Do you suggest to treat differently the two syntaxes "M.(e)" and "let open M in e" ?

I'm not sure about that and don't really have an opinion on it. let open is nearer to open and I think that the pattern you're most likely to use is something like:

let f () =
  let open M in
  let id = ... in
  let c = ... in
  id + c

which will do the right thing most of the time. I think that M.(e) is a little bit particular since you use it in expressions themselves after you have declared your local variables.

But yes the warning on M.(id + c) should be something like "`id` refers to M.id and not this local variable".

Warnings about arbitrary ambiguous references I'm not sure, people may do that by design. Myself I usually don't open to "import" values though and treat it as a bad practice. I only open to "import" modules.

> Did you enable warning 27?

No, I usually take the default set warnings (which I expect to be the good practice and maybe because I'm isolated from the concrete command line tools by ocamlbuild these days).
(0009064)
frisch (developer)
2013-04-10 14:25

> I think that the pattern you're most likely to use is something like:

The following will also be quite common:

 let f id =
    let open M in
    let c = ... in
    id + c

> Warnings about arbitrary ambiguous references I'm not sure, people may do that by design.

It's quite common to have open statements at the top of the file, in which case they cannot override local identifiers. This could be a more specific warning (only shadowing local identifiers, not identifiers imported through other open statements). But even the case where two open statements introduce a common name (used in the rest of the unit) is a quite fragile pattern.
(0009065)
dbuenzli (reporter)
2013-04-10 14:32

I agree with both comments (for the second one I was initially confused by the difference between open and include).
(0009067)
gasche (developer)
2013-04-10 15:23

Could you or Alain comment on exactly which kind of shadowing would result in a warning, and which one wouldn't?

One problem is that some people use open with the explicit purpose of shadowing identifiers existing in the context:

    open List
    open ExtList (* contains a List submodule that redefines List.map *)

Alain makes a distinction when he says that open statements at the top of a file cannot override *local* identifier. I suppose one could claim here that List.map is not local, and therefore can be shadowed without explicit warning. But what about

   module List = struct
     let map = ...
   end
   open ExtList

are we shadowing a local value? Is there any semantic difference between this and the case where List a reference to an external module? More generally, it seems easy to transform a bunch of 'let' into an 'open' as a reasonable refactoring step.

In any case, we might do the following distinction:
- an 'open' shadowing an 'open' corresponds to a pattern used in practice so it shouldn't be included in the warning
- an 'open' shadowing a 'let' is probably dangerous and could result in a warning
- a 'let' shadowing a 'let' is probably intentional (it's common in some coding patterns) and should not result in a warning

I assume other single-variable binders (patterns, for loop indices, function or functor parameters...) should be handled like 'let' bindings. What about other way(s) to introduce binders in scope, namely 'include'? Should it be handled like 'open'? Or like 'let'? Or have a hierarchy of "more explicit" than (let+fun > include > open), with a warning when a less explicit binding shadows a more explicit binding?
(0009069)
dbuenzli (reporter)
2013-04-10 16:10

For me we could drop the idea of having a warning at the toplevel. My concern is really about this M.() notation where I think the problem is really acute.
(0009070)
frisch (developer)
2013-04-10 16:16

I don't get your example with "module List = struct ... end open ExtList" (there is no shadowing).

> - an 'open' shadowing an 'open' corresponds to a pattern used in practice so it shouldn't be included in the warning

I'd rather have it as a *different* warning. Some people do it on purpose (and they will disable the warning), but I find it a bad style to have your code depend on the ordering between opens.

> - an 'open' shadowing a 'let' is probably dangerous and could result in a warning

Yes.

> - a 'let' shadowing a 'let' is probably intentional (it's common in some coding patterns) and should not result in a warning

We should probably be lazy here. Since the idiom is rather common and well accepted, I don't see a reason to create a warning unless someone explicitly asks for it. (I can imagine some local coding guidelines forbidding the idiom.)

> What about other way(s) to introduce binders in scope, namely 'include'?

(or "inherit" in objects)

I don't have a strong opinion here. What do you think?
(0009071)
gasche (developer)
2013-04-10 17:02
edited on: 2013-04-10 17:03

I agree with 'open' shadowing 'open' deserving a warning of its own.

Daniel: I see no good reason to have a different behavior between toplevel bindings and non-toplevel bindings here -- at least if there exists a consensus that gracefully extends to toplevel bindings.

Alain: my personal reasoning is that all introduction form that explicitly (lexically) contains the identifiers they introduce should be considered equivalent and "very explicit", and all introduction forms whose set of bound identifiers are only known at type-checking time should be considered equivalent and "less explicit".


A last remark is that I expect these "local open" to be fairly useful to get a form of operator overloading -- in fact the syntax was inspired by the the "pa_do" extension. Code such as

let foo = ...
  Int32.(1l + 2l)

(or possibly Int32.(1l +. 2l) depending on the lexical conventions)

In this case 'open' overloading 'let' might happen for such infix operators. Another potential example:

  let (>>=) = filter_map

  module Transformer (M : MONAD) = struct
    type 'a m = 'a M.m list
    let bind m f =
      m >>= fun elem ->
      M.(elem >>= fun v -> f v)
    let return = ...
  end

I think the robustness benefits of protecting users from unwanted open shadowing outweigh the cost of sacrificing this (so far little used) case, but it's however something to keep in mind.

(0009072)
dbuenzli (reporter)
2013-04-10 17:28

> Daniel: I see no good reason to have a different behavior between toplevel bindings and non-toplevel
> bindings here -- at least if there exists a consensus that gracefully extends to toplevel bindings.

No problem. It's more that I don't have a lot of time to think about this at the moment. I'm confident you'll find something sensible.

> A last remark is that I expect these "local open" to be fairly useful to get a form of operator overloading ->- in fact the syntax was inspired by the the "pa_do" extension. Code such as

> let foo = ...
> Int32.(1l + 2l)

Mmmh in fact that's the exact use case were I fell on the issue. I use this (but quite conservatively, I think too much operators kills readability) in gg (https://github.com/dbuenzli/gg [^]) to allow vectors to be added, subtracted and multiplied by scalars.

  let v = ... in
  let u = ... in
  V2.(3 * u + v)

You mean that we'd get a warning here on the *, + ? I wouldn't like it...
(0009073)
gasche (developer)
2013-04-10 17:38

The way I understand the current proposal, you would only get a warning if the (*), (+) already present in scope where introduced by an explicit let-binding in the lexical scope (open-shadows-let), not if they are themselves the result of an previous 'open' -- in fact you would get a warning, but a distinct one, open-shadows-open, that would be disabled by default.

(In the open-shadows-open case, people may still want to distinguish shadowing of infix operators and shadowing of usual operators. That may be going a bit too far...)
(0009074)
frisch (developer)
2013-04-10 18:57

We should also think about how a user who enabled the new warnings to silence them locally (without such a feature, people with tend to disable the warning altogether). This could be achieved by another feature which would be useful on its own: removing an identifier from the current scope (with both a local and a structure-level version). Any clever idea of syntax for such an "unlet" construction?
(0009075)
gasche (developer)
2013-04-10 21:22

Another remark: the way our Haskell and Agda friends handle shadowing is a bit different. If you open two modules Foo and Bar that export the same identifier 'x', you get no reaction while 'x' is unused, but you get an error as soon as you actually try to use 'x'.
I think the way to think about it is that they consider that 'open' should be reorderable at will, and that a common identifier is not shadowed but becomes "ambiguous". Making identifiers ambigous is not an issue, but using them is frowned upon.

Do you think either warning (open-shadows-let or open-shadows-open) would benefit from being activated only at the place of ambiguous use, rather than the ambiguity-creating binding?
(0009076)
dbuenzli (reporter)
2013-04-10 21:51

> Do you think either warning (open-shadows-let or open-shadows-open) would benefit from being activated only at the place of ambiguous use, rather than the ambiguity-creating binding?

I think the answer is yes but I'm not sure I understand correctly what you mean. If I take gg as an example V2.ox exists. Now if I write:
  
  let ox = ... in
  let v = ... in
  let u = ... in
  V2.(3 * u + v), ox

I wouldn't expect a warning. But in the following case, I would expect a warning on the use of ox in V2.():

  let ox = ... in
  let v = ... in
  let u = ... in
  V2.(3 * u + v - ox)
(0009077)
gasche (developer)
2013-04-10 21:59

Exactly, that's what I meant. In the first example, open V2 shadows `ox`, so could result in a warning. I'm suggesting to warn only if the shadowing identifier is actually used.
(0009079)
frisch (developer)
2013-04-10 22:47

Gabriel: yes, I believe that if we implement such a warning, it must be triggered upon ambiguous references (that's what I was referring to by "referring to an identifier imported in the current scope by some open, but which was already present in the environment before that open" in my first note), not upon shadowing itself. Shadowing is quite common, especially on type names (often just "t"), and I don't see it as a problem as long as the ambiguous name is not used.
(0009081)
lpw25 (developer)
2013-04-11 10:26

> We should also think about how a user who enabled the new warnings to silence them locally (without such a feature, people with tend to disable the warning altogether)

I think these warnings would be best silenced in the same way as the method override warnings:

  let map = ...
  open List
  let f = map (* emits warning *)

vs

  let map = ...
  open! List
  let f = map (* no warning *)

This could also work for a warning on let-based shadowing:

  let x = 3 in
  let x = 4 in (* emit warning *)

vs

  let x = 3 in
  let! x = 4 in (* no warning *)

Just like "method!" these versions would give errors if they did not shadow any variables.

Note that this is why "let!" was rejected as a possible syntax for monadic binds.
(0009277)
frisch (developer)
2013-05-13 16:37

> I think these warnings would be best silenced in the same way as the method override warnings:

What about the M.(e) syntax?

I think an explicit way to "undefine" an identifier would be useful on its own, not only to silence the shadowing warning in a more fine-grained way. For instance, a Camlp4 extension or a ppx rewriter might want to introduce global shared values accessible to a whole compilation unit (e.g. such as transition tables for "sedlex") without exporting them to the inferred signature for the compilation unit.
(0009278)
lpw25 (developer)
2013-05-13 16:54

> What about the M.(e) syntax?

I guess that would either require an ugly "M.!(e)" syntax, or people shadowing let bound variables should switch to "let open! M in e".

> I think an explicit way to "undefine" an identifier would be useful on its own, not only to silence the shadowing warning in a more fine-grained way. For instance, a Camlp4 extension or a ppx rewriter might want to introduce global shared values accessible to a whole compilation unit (e.g. such as transition tables for "sedlex") without exporting them to the inferred signature for the compilation unit.

I agree, but I still think an "overriding" open syntax is a useful way to avoid the warning. I particularly think it would be needed if a "'let' shadowing a 'let'" warning were introduced.
(0009285)
frisch (developer)
2013-05-16 16:01

I've added (commit 13683) a new warning (44) to report open statements which shadow an identifier which is later used. Commit 13684 introduces the syntax "open! M" to silence the warning.

A subtle case is triggered by the test suite. Consider:

module M1 = struct
  type t = {x: int; y: int}
  type u = {x: bool; y: bool}
end

open M1
let f1 (r:u) = r.x

A warning is reported:

Warning 44: this open statement shadows the label identifier x (which is later used)

because we refer to the label u.x introduced by the open statement and it shadows the other label x (also introduced by the same open statement). If we replace "(r:u)" by "(r:t)", no warning is given.

Do you guys think we need to support this case (open statement introducing labels/constructors shadowing other items from the same open) to avoid reporting a warning? Or maybe we should simply restrict the warning to value/type/... identifiers, but exclude labels and constructors (now that we have type-based disambiguation)?
(0009286)
dbuenzli (reporter)
2013-05-16 16:36

Well since there's no warning in M1 itself it fells a little bit strange to get a warning in the case you mention.
(0009287)
frisch (developer)
2013-05-16 19:17

Let me explain what's going on in this example.

The new warning is implemented by modifying the way components are inserted in the environment while type-checking an "open" statement. Concretely, an element X inserted because of an "open" statement is instrumented so that, when X will be accessed, it will be checked if a component of the same name X (and kind) already existed in the previous environment (before the insertion of this X).

Now in the example, type-checking "open M1" will have the effecth that "x" will be inserted twice in a row in the environment. The second one (from type u) will remember that it shadows an existing field "x" (from type t), already inserted in the environment.

We could fix that by checking shadowing w.r.t. the environment before beginning the type-checking of the "open". But now labels and constructors can be accessed through their type definition, it might just make more sense to not report any shadowing warning for them. This will be an easier fix, and it will also avoid warnings in cases like:

module M1 = struct type t = {x: int} end
module M2 = struct type t = {x: int} end

open M1
open M2

let f (r : M2.t) = r.x
(0009288)
frisch (developer)
2013-05-17 13:41

FWIW, the warning no longer reports shadowing between two identifiers introduced in the current environment by the same "open" statement. I'm still interested in hearing your feedback concerning keeping or dropping the warning for labels and constructors (because even if they are shadowed, they are still accessible).
(0009289)
hcarty (reporter)
2013-05-17 14:51

Having a separate warning for shadowing constructors and labels could be useful for people who have the new label/constructor ambiguity warnings enabled. If you don't want to rely on type-based disambiguation it would be useful to know when a module open introduces new shadowing.

Is there a warning for shadowed modules?
(0009290)
frisch (developer)
2013-05-17 14:55

> Is there a warning for shadowed modules?

Yes, the same warning currently reports shadowing for all kinds of identifiers (values, types, modules, module types, classes, class types, labels, constructors).

It could make sense, indeed, to provided different warnings to cover each category.
(0009541)
frisch (developer)
2013-06-18 10:01

Split the warning in 2: 45 for label and constructors; 44 for other kinds of identifiers. Commit 13796 on 4.01, 13797 on trunk.
(0010307)
hcarty (reporter)
2013-09-04 16:35

Is there a M.( e ) version of the open! syntax available? Switching to "let open! M in e" is an option sometimes but it makes it more difficult/verbose to restrict the open to 'e'.
(0010966)
dbuenzli (reporter)
2014-02-20 20:54

Is there any strong reason why this warning is not enabled by default ?

I thought it was and just got caught again (and AFAIK ocamlbuild has no easy way of setting up warnings, unless the dreaded myocamlbuild.ml).
(0010967)
gasche (developer)
2014-02-20 22:13

> AFAIK ocamlbuild has no easy way of setting up warnings, unless the dreaded myocamlbuild.ml

I do see through the technique of dropping unrelated comments to get potential contributors that read those things into action (re. stack traces: please send a patch!).

As explained in this blog post

  http://gallium.inria.fr/blog/quick-tip-the-ocamlbuild-documentation-option/ [^]

the -documentation flag is your friend (... for non-parametrized flags).

  ocamlbuild -documentation | grep warn

There is also a warn(X) parametrized flag and, only in trunk, also a warn_error(X) one.
(0010968)
dbuenzli (reporter)
2014-02-20 23:10

Thanks about the -documentation tip I always forget that, I'm always heading for that outdated ocamlbuild wiki. But in that case yes I need the parametrized tag.

Getting back on topic actually having it by default in its current state makes it unusable for custom operators which in fact is one of the advantages of the M.() notation. For example it complains about the following usage.

module V = struct
  type t = int * int
  let ox = (1, 0)
  let oy = (0, 1)
  let dot (x0, y0) (x1, y1) = x0 * x1 + y0 * y1
  let add (x0, y0) (x1, y1) = (x0 + x1, y0 + y1)
  let ( + ) = add
end

let _ =
  let a = (3, 2) in
  let b = (3, 4) in
  V.(a + b)

Warning 44: this open statement shadows the value identifier + (which is later used)

Now I don't want it to complain in that case. I'm more interested though is that it complain about regular identifier like in the example below (a similar example cost me a few hours of numerical code debugging today).

let _ =
  ...
  let ox = V.(b - a) in
  V.(dot ox (d - c))

I don't know should I live dangerously or should the warning maybe take the very ad hoc solution of treating operators differently ?

- Issue History
Date Modified Username Field Change
2013-04-10 13:05 dbuenzli New Issue
2013-04-10 13:13 frisch Note Added: 0009059
2013-04-10 14:11 dbuenzli Note Added: 0009063
2013-04-10 14:25 frisch Note Added: 0009064
2013-04-10 14:32 dbuenzli Note Added: 0009065
2013-04-10 15:23 gasche Note Added: 0009067
2013-04-10 16:10 dbuenzli Note Added: 0009069
2013-04-10 16:16 frisch Note Added: 0009070
2013-04-10 17:02 gasche Note Added: 0009071
2013-04-10 17:03 gasche Note Edited: 0009071 View Revisions
2013-04-10 17:28 dbuenzli Note Added: 0009072
2013-04-10 17:38 gasche Note Added: 0009073
2013-04-10 18:57 frisch Note Added: 0009074
2013-04-10 21:22 gasche Note Added: 0009075
2013-04-10 21:51 dbuenzli Note Added: 0009076
2013-04-10 21:59 gasche Note Added: 0009077
2013-04-10 22:47 frisch Note Added: 0009079
2013-04-11 10:26 lpw25 Note Added: 0009081
2013-04-15 12:04 gasche Status new => confirmed
2013-05-13 16:37 frisch Note Added: 0009277
2013-05-13 16:54 lpw25 Note Added: 0009278
2013-05-16 16:01 frisch Note Added: 0009285
2013-05-16 16:01 frisch Assigned To => frisch
2013-05-16 16:01 frisch Status confirmed => assigned
2013-05-16 16:36 dbuenzli Note Added: 0009286
2013-05-16 19:17 frisch Note Added: 0009287
2013-05-17 13:41 frisch Note Added: 0009288
2013-05-17 14:51 hcarty Note Added: 0009289
2013-05-17 14:55 frisch Note Added: 0009290
2013-06-18 10:01 frisch Note Added: 0009541
2013-06-18 10:01 frisch Status assigned => resolved
2013-06-18 10:01 frisch Fixed in Version => 4.01.0+dev
2013-06-18 10:01 frisch Resolution open => fixed
2013-09-04 16:35 hcarty Note Added: 0010307
2014-02-20 20:54 dbuenzli Note Added: 0010966
2014-02-20 22:13 gasche Note Added: 0010967
2014-02-20 23:10 dbuenzli Note Added: 0010968


Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker