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
Meta-issue on ambiguity warnings #6951
Comments
Comment author: @lpw25 To add to the list of possible ideas, it might be useful to separate the case of an open shadowing an identifier that was introduced by an open (including the initial open of Pervasives) from the case of an open shadowing an identifier that was introduced by a let. The second case seems much more likely to be a bug than the first one. |
Comment author: @lpw25 Another suggestion: Add warnings (and corresponding !-forms) for other causes of shadowing, especially include and let. |
Comment author: @Drup I like the idea of distinguishing between variables introduce by an open/include and by a let. |
Does this issue include warning on let shadowing or is it only related to shadowing from open/import? I am interested in a warning that would warn of let shadowing of a variable in the same scope to prevent the accidental use of an existing variable name. If the type was different then that would potentially catch the problem, but if the types were the same then a warning would be nice to catch these types of mistakes. let specialValue = "someImportantValue"
foo specialValue
...
(* further down in the file, we make this change, totally unrelated change *)
let specialValue = "differentValue" (* this had no relation to the original
specialValue, we just hadn't realized this name was used *)
cat specialValue
...
(* later in the file *)
bar specialValue (* now this is broken was expecting someImportantValue *) |
I'm a bit uncomfortable with a warning on let-shadowing, as reusing the same name can be a good idea. For instance, in my experience the following code is both safe and readable: let f env =
let env = do1 env in
let env = do2 env in
do3 env In particular I had some bad experiences in the past with using different names, and referring to the bad one at some point (unused variable warning can help, but only to some extent). On the other hand, doing this on toplevel bindings in a file is less common, and we might want to avoid it for readability. |
Yes, I agree that let-shadowing for local expressions is a normal use, so maybe the warning I am asking for would be for non-local expression let binding shadowing. This would occur when you are doing global let binding or at the let binding at the top scope of a function (not part of a local expression). As I understand it, being a warning it could be configured on/off depending on the user's desires. The idea would be to let the compiler help catch situation where a user accidentally reuses a variable name which could introduce a bug without warning if the type matches. |
I more than agree with you; in loops or recursive functions, I often use,
```ocaml
let i = i + 1 in
```
after having troubles with useless auxiliary names as in let j = i + 1 ...
that allows easy confusion between i and j.
Same story for toplevel re-binding, I often redefined Stdlib functions to
slightly change their meaning either in the toplevel scope of a module, or to
export them to the external world. Or simply because I want to use some Stdlib
name (e.g. algebraic operations +, *, ...).
… > Does this issue include warning on let shadowing or is it only related to shadowing from open/import?
>
> I am interested in a warning that would warn of let shadowing of a variable in the same scope to prevent the accidental use of an existing variable name.
I'm a bit uncomfortable with a warning on let-shadowing, as reusing the same name can be a good idea. For instance, in my experience the following code is both safe and readable:
```ocaml
let f env =
let env = do1 env in
let env = do2 env in
do3 env
```
In particular I had some bad experiences in the past with using different names, and referring to the bad one at some point (unused variable warning can help, but only to some extent).
On the other hand, doing this on toplevel bindings in a file is less common, and we might want to avoid it for readability.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#6951 (comment)
|
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. |
Original bug ID: 6951
Reporter: @gasche
Status: acknowledged (set by @gasche on 2015-08-04T09:52:31Z)
Resolution: open
Priority: normal
Severity: feature
Version: undecided
Target version: undecided
Category: typing
Parent of: #5759 #5980 #6023 #7386
Monitored by: @Drup @diml @hcarty @dbuenzli
Bug description
This meta-issue is meant to reference, discuss and inform the past,
current and future feature suggestions related to handling of
ambiguity, in particular warnings when the compiler detects an
ambiguity situation.
The compiler may emit warning when it makes a choice on the meaning of
a particular identifier or symbol that may not be what the user
intents. Those warnings are intended to:
choices
new type annotation changing the ambiguity resolution of a field
name from scope-based to type-based; a new declaration in an opened
module shadowing a local identifier that was previously used, etc.)
Warnings are meant to flag situations that are likely sources of those
issues. The set of warnings should have good defaults that steer code
authors towards what is consensually considered good style, but also
be flexible enough to allow more or less demanding users to authorize
or rule out some slightly different styles.
Feel free to discuss problems with the current state of the ambiguity
detection, or propose improvements. If an idea gets a substantial
consensus, it should have an issue of the bugtracker on its own.
Additional information
The list of ideas currently floating across the bugtracker, caml-list
and github pull requests is the following:
[separate symbol warning]: #5980 discusses the problem that we
people may want to allow shadowing of symbols (infix and prefix
operators) but warn on shadowing of alphabetic identifiers. The
current shadowed-by-open warning (44) should be split between the
alphabetic name case and the symbol case.
[aliasing redefinition]: #5980 suggests that we should have no
warning when the shadowed and the shadowing identifier point to
the same thing
[M.!()]: #218 proposes a "M.!( ... )" syntax as a counterpart to
"let open! M in ..."
#218
[incompatible types]: on caml-list Peter Urkedal suggests to only
warn when the type of the shadowed and shadowing identifiers are
compatible (one can be used in place of the other without breaking
type-correctness); if they incompatible, the user cannot
intentionally use one instead of the other.
https://sympa.inria.fr/sympa/arc/caml-list/2015-08/msg00027.html
The text was updated successfully, but these errors were encountered: