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

Meta-issue on ambiguity warnings #6951

Closed
vicuna opened this issue Aug 4, 2015 · 9 comments
Closed

Meta-issue on ambiguity warnings #6951

vicuna opened this issue Aug 4, 2015 · 9 comments

Comments

@vicuna
Copy link

vicuna commented Aug 4, 2015

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:

  • make the code more readable by avoiding "surprising" disambiguation
    choices
  • make the code robust to internal changes or dependency changes (a
    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

@vicuna
Copy link
Author

vicuna commented Aug 4, 2015

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.

@vicuna
Copy link
Author

vicuna commented Aug 4, 2015

Comment author: @lpw25

Another suggestion: Add warnings (and corresponding !-forms) for other causes of shadowing, especially include and let.

@vicuna
Copy link
Author

vicuna commented Aug 7, 2015

Comment author: @Drup

I like the idea of distinguishing between variables introduce by an open/include and by a let.

@vicuna
Copy link
Author

vicuna commented Aug 16, 2015

Comment author: @gasche

I propose a GPR implementing [separate symbol warning] in

#232

@jeffbski
Copy link

jeffbski commented Nov 18, 2019

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

@garrigue
Copy link
Contributor

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:

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.

@jeffbski
Copy link

jeffbski commented Nov 19, 2019

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.

@pierreweis
Copy link
Contributor

pierreweis commented Nov 26, 2019 via email

@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

4 participants