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 on use of an identifier shadowed by module open -- feature implemented, but refinements still possible #5980

Closed
vicuna opened this issue Apr 10, 2013 · 35 comments

Comments

@vicuna
Copy link

vicuna commented Apr 10, 2013

Original bug ID: 5980
Reporter: @dbuenzli
Assigned to: @alainfrisch
Status: acknowledged (set by @gasche on 2016-10-14T20:19:58Z)
Resolution: reopened
Priority: normal
Severity: feature
Version: 4.00.1
Fixed in version: 4.01.0+dev
Category: typing
Has duplicate: #6428
Related to: #7386
Child of: #6951
Monitored by: @jmeber @hcarty

Bug description

While 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

Additional information

[Gabriel Scherer]: I reopened this bug to track the suggestion from Peter Urkedal reported in

#5980#c14317

@vicuna
Copy link
Author

vicuna commented Apr 10, 2013

Comment author: @alainfrisch

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.)

@vicuna
Copy link
Author

vicuna commented Apr 10, 2013

Comment author: @dbuenzli

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).

@vicuna
Copy link
Author

vicuna commented Apr 10, 2013

Comment author: @alainfrisch

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.

@vicuna
Copy link
Author

vicuna commented Apr 10, 2013

Comment author: @dbuenzli

I agree with both comments (for the second one I was initially confused by the difference between open and include).

@vicuna
Copy link
Author

vicuna commented Apr 10, 2013

Comment author: @gasche

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?

@vicuna
Copy link
Author

vicuna commented Apr 10, 2013

Comment author: @dbuenzli

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.

@vicuna
Copy link
Author

vicuna commented Apr 10, 2013

Comment author: @alainfrisch

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?

@vicuna
Copy link
Author

vicuna commented Apr 10, 2013

Comment author: @gasche

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.

@vicuna
Copy link
Author

vicuna commented Apr 10, 2013

Comment author: @dbuenzli

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...

@vicuna
Copy link
Author

vicuna commented Apr 10, 2013

Comment author: @gasche

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...)

@vicuna
Copy link
Author

vicuna commented Apr 10, 2013

Comment author: @alainfrisch

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?

@vicuna
Copy link
Author

vicuna commented Apr 10, 2013

Comment author: @gasche

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?

@vicuna
Copy link
Author

vicuna commented Apr 10, 2013

Comment author: @dbuenzli

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)

@vicuna
Copy link
Author

vicuna commented Apr 10, 2013

Comment author: @gasche

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.

@vicuna
Copy link
Author

vicuna commented Apr 10, 2013

Comment author: @alainfrisch

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.

@vicuna
Copy link
Author

vicuna commented Apr 11, 2013

Comment author: @lpw25

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.

@vicuna
Copy link
Author

vicuna commented May 13, 2013

Comment author: @alainfrisch

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.

@vicuna
Copy link
Author

vicuna commented May 13, 2013

Comment author: @lpw25

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.

@vicuna
Copy link
Author

vicuna commented May 16, 2013

Comment author: @alainfrisch

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)?

@vicuna
Copy link
Author

vicuna commented May 16, 2013

Comment author: @dbuenzli

Well since there's no warning in M1 itself it fells a little bit strange to get a warning in the case you mention.

@vicuna
Copy link
Author

vicuna commented May 16, 2013

Comment author: @alainfrisch

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

@vicuna
Copy link
Author

vicuna commented May 17, 2013

Comment author: @alainfrisch

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).

@vicuna
Copy link
Author

vicuna commented May 17, 2013

Comment author: @hcarty

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?

@vicuna
Copy link
Author

vicuna commented May 17, 2013

Comment author: @alainfrisch

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.

@vicuna
Copy link
Author

vicuna commented Jun 18, 2013

Comment author: @alainfrisch

Split the warning in 2: 45 for label and constructors; 44 for other kinds of identifiers. Commit 13796 on 4.01, 13797 on trunk.

@vicuna
Copy link
Author

vicuna commented Sep 4, 2013

Comment author: @hcarty

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'.

@vicuna
Copy link
Author

vicuna commented Feb 20, 2014

Comment author: @dbuenzli

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).

@vicuna
Copy link
Author

vicuna commented Feb 20, 2014

Comment author: @gasche

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.

@vicuna
Copy link
Author

vicuna commented Feb 20, 2014

Comment author: @dbuenzli

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 ?

@vicuna
Copy link
Author

vicuna commented May 5, 2014

Comment author: @gasche

Another tweak that may make sense for the patch is to allow shadowing (without warning) in the case where both identifiers have the same long path (they are in fact the same identifier). I've just had an ambiguity warning because M.(...) is used locally in a module that has a toplevel "open M", and I found it quite surprising.

A use-case is a function that does several local "M.(...)" or "let open M" for the same module M, and decides to change to use only a single "let open M" at the top of the expression. With the current behavior, transitory versions (with both the local opens and the global open) raise an ambiguity warning, which is unpleasant.

module A = struct
let x = 1
let y = 2
end

let first_version a b = (a - A.(x + y), b - A.(x * y))
let final_version a b = A.(a - (x + y), b - (x * y))

(* warns *)
let transitory_version a b = A.(a - A.(x + y), b - A.(x * y))

Of course that is only a minor aspect. The discussion of a difference between (let open M in ...) and (M.(...)) is probably more important usability-wise.

@vicuna
Copy link
Author

vicuna commented May 7, 2014

Comment author: @alainfrisch

in the case where both identifiers have the same long path

Agreed. I've encountered this situation in practice. Feel free to propose a patch!

@vicuna
Copy link
Author

vicuna commented Aug 4, 2015

Comment author: @gasche

On the caml-list
https://sympa.inria.fr/sympa/arc/caml-list/2015-08/msg00027.html
Peter Urkedal has an excellent suggestion:

This suggests another option. If type information is available at
the point this warning is emitted, then the warning could be issued
only in the case when the type of the shadowing identifier matched
that of the shadowed identifier.

This assumes the common case for shadowing is to redefine operators
or common functions at a custom type, the use of M.() being an
alternative to overloading. Loosely the warning should be emitted
if the chosen identifier is not the one which would have been chosen
by some sensible overloading scheme, but instead we make a simple
estimate.

This could still go wrong, since the type required by the context
may be general than the type of both the shadowed and shadowing
terms, so a better rule might be to issue the warning if both are
admissible in the given context, though my guess is that's harder to
implement.

@vicuna
Copy link
Author

vicuna commented Aug 4, 2015

Comment author: @dbuenzli

Gabriel, could you reopen the bug to make sure we don't forget or should we open a new one ?

@vicuna
Copy link
Author

vicuna commented Oct 14, 2016

Comment author: @gasche

Peter Urkedal's suggestion would have resolved the small issue I report in #7386.

@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

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

2 participants