Better error reporting in case of missing 'rec' in let-bindings by Armael · Pull Request #1472 · ocaml/ocaml

Armael · 2017-11-10T22:09:48Z

This is a first attempt at splitting "easy" and self-contained bits of PR #102 by @charguer, and follows an offline discussion involving @charguer, @gasche, @let-def and myself.
As described in http://chargueraud.org/research/2015/ocaml_errors/ocaml_errors.pdf (section 5, Message for missing "rec"), the purpose of the code extracted in this PR is to provide better error reporting for the case of a forgotten rec keyword.

In essence, what this PR does is additionally track in the environment "ghost" bindings for the non-recursive let-bindings that have been encountered. Then, in case of an unbound identifier, we additionally check if this identifier is present as a ghost binding in the environment. If this is the case, we display a error message indicating that a rec keyword may be missing at the binding site of this identifier.

In Arthur's original patch (and in the first commit of this PR), ghost bindings were implemented by adding a special string marker at the beginning of normal identifier. As this is relatively fragile, I rewrote this mechanism by adding a new kind of "ghost" bindings to typing environments.

This is my first dive in this part of OCaml's source code, so it is very possible that this patch contains rookie mistakes. I'll be happy to take remarks and criticism :-) .

gasche

I haven't done a full review of the patch yet. Two remarks:

I think it would be nice if the patchset was nice. Here you have Arthur's ugly patch first, and then a more general approach as a diff over it. Maybe it would be possible to add the more general mechanism first, and then implement Arthur's idea on top of it? This impacts the code: in 293b522 you document your Env_ghost_value by saying that it is about let rec, but this is wrong, the explanation is over-specialized. This should be a more generic concept.

You chose to implement "ghost value bindings" as one extra kind of environment. I find it surprising as I would rather have expected that you would add a Val_ghost kind to the "value kinds" stored in the value environment. I don't know which of the two approaches is best, but I think it deserves discussion.

Having a separate environment means that there is no ordering between value bindings and ghost value bindings. Having a single environment means that ghost bindings will shadow value bindings of the same name and conversely. In your case it doesn't really matter as the "unbound identifier" error will only arise if there are no normal-value-bindings for that name, but for other uses of ghost bindings, what would be the better behavior?

(One thing that is a bit strange about having it as an extra part of the environment is that it is only about value names, not type names or module names.)

gasche · 2017-11-10T23:46:53Z

typing/env.ml

    comp_components = Tbl.empty; comp_classes = Tbl.empty;
-    comp_cltypes = Tbl.empty }
+    comp_cltypes = Tbl.empty;
+    comp_ghost_values = Tbl.empty; }


You should break a line before } to make future additions a one-line diff.

gasche · 2017-11-10T23:47:17Z

typing/env.ml

          comp_components = Tbl.empty; comp_classes = Tbl.empty;
-          comp_cltypes = Tbl.empty } in
+          comp_cltypes = Tbl.empty;
+          comp_ghost_values = Tbl.empty; } in


line break before }

Armael · 2017-11-11T12:45:18Z

Implementing "ghost value bindings" as a different value kind should indeed result in a cleaner and less invasive patch. I'll rebase the patch and do that.

Armael · 2017-11-11T13:26:24Z

However, with this way of doing things it seems harder to convince myself that I don't break things: with a separate environment, it's impossible for existing parts of the code to look up ghost bindings by mistake. With a different value kind, it seems possible or even likely for the new "ghost" kind to be matched by a wildcard pattern somewhere and handled as something else.

gasche · 2017-11-11T13:40:42Z

I guess that git grep -C 20 " Val_" is your friend. But I think it would be nice to find another use-case to have in mind to know whether the way you adapt the code makes sense. (And possibly another use-case to test again.) Ghost values seem like a generally useful thing to have, can we find a second use-case?

Note that there is also Val_unbound that plays a not-completely-different role. I looked (as a result of running the grep above) and it seems useful to hide some object-system variables from other part of the code, and generates a custom error message (currently object-system-specific) when it is found during lookup. Maybe we actually want to generalize Val_unbound with a reason (instance variable, ghost recursive variable, etc.) instead of adding a second kind? (In that case most places don't have to change, although I doubt that they would actually do the right thing today if they were reached by a Val_unbound binding.)

shindere · 2017-11-11T14:28:18Z

Armael (2017/11/11 13:26 +0000):

However, with this way of doing things it seems harder to convince myself that I don't break things: with a separate environment, it's impossible for existing parts of the code to look up ghost bindings by mistake. With a different value kind, it seems possible or even likely for the new "ghost" kind to be matched by a wildcard pattern somewhere and handled as something else.

Sure, but having a separate environment does, IMO, not scale. You wouldn't imagine adding one new kind of environment for every different kind of value you'd want to handle, would you? To prevent the kind of problem you are mentionning, i.e. exhaustive pattern illegitimately matching your ghost values, isn't it okay to simply get rid of all the exhaustive patterns in the code? I didn't look into it myself so no idea whether this is realistic, but I think we have a warning that can be turned into an error to have the compiler verifying such kind of matchings do not occur, right?

Armael · 2017-11-11T14:41:05Z

Sure, but having a separate environment does, IMO, not scale. You wouldn't imagine adding one new kind of environment for every different kind of value you'd want to handle, would you?

Yes, I agree it was a bad idea.

I like the idea of reusing Val_unbound, so I'll try that next.

Armael · 2017-11-11T17:58:55Z

Having a single environment means that ghost bindings will shadow value bindings of the same name and conversely. In your case it doesn't really matter [...]

By playing with the idea of reusing Val_unbound, I just realized this is actually an issue. Consider the following piece of code:

let x = 1 in
let x = x+1 in
()

Adding a ghost binding in the environment of the lhs of the second let (x+1) will shadow the outer regular binding x = 1, and then fail, producing a "missing rec" error message.

gasche · 2017-11-11T18:12:00Z

You have to check that no identifier of that name already exists in scope if you want to add a ghost/unbound binding for it. If a name already exists, there is no point in adding the ghost binding anyway, as there will not be an unbound-identifier error later.

Armael · 2017-11-13T20:27:14Z

I pushed a new patchset, based on the idea of reusing Val_unbound. A non-trivial part of the patch consists in extending Env.lookup_* functions to allow disabling "marking", in order to implement @gasche suggestion in the previous comment.
Indeed, one wants to add a ghost binding to the environment only if the name is not bound already. For this, one needs to look up a value in the environment, without triggering the side-effects tracking which bindings/modules are used (in order to print warnings "value foo is not used"). Consequently, the second commit of this patch extends the lookup functions to take an additional optional argument, specifying whether marking should be done (and the default value reflects the current behavior).

Armael · 2017-11-13T20:48:48Z

Umm, travis seems to have failed, but I have no idea why?

Octachron · 2017-11-13T21:12:05Z

Travis is only failing due to the lack of new change entry in Changes.

Armael · 2017-11-13T21:13:34Z

Thanks. I added a Changes entry.

gasche · 2017-11-13T22:38:41Z

@alainfrisch, I think we need your opinion on the ?mark:bool parameter on lookup functions. Is this something that you agree is a reasonable design? It's a bit invasive, but my intuition is that we would have needed it eventually anyway, so adding this explicit control now is a good idea.

alainfrisch · 2017-11-15T08:37:37Z

I think we need your opinion on the ?mark:bool parameter on lookup functions. Is this something that you agree is a reasonable design? It's a bit invasive, but my intuition is that we would have needed it eventually anyway, so adding this explicit control now is a good idea.

Yes, I think it is reasonable, the code looks good, and I agree there might be other reasons in the future to need the flag anyway. I'm only wondering whether we shouldn't make the parameter non-optional. It'd be even more invasive (and not really related to the goal of this PR), but this might catch instances where marking is actually not desired, and at least make things more explicit.

Armael · 2017-11-15T08:53:42Z

I'm only wondering whether we shouldn't make the parameter non-optional. It'd be even more invasive (and not really related to the goal of this PR), but this might catch instances where marking is actually not desired, and at least make things more explicit.

I could submit an other PR that would do that (but maybe after this one is merged?).

shindere · 2017-11-15T09:40:53Z

Just an out-of-curiosity question. Would it make sense to have two families of lookup functions, one that marks and one that does not?

alainfrisch · 2017-11-15T10:53:08Z

I could submit an other PR that would do that (but maybe after this one is merged?).

Yes, ok, no need to "pollute" this PR.

Would it make sense to have two families of lookup functions, one that marks and one that does not?

There is a risk of combinatorial explosion if we ever need to introduce other flags on the lookup mechanism. And even if we don't, having a single val lookup: mark:bool -> ... seems nicer to me than val lookup_mark: ... and val lookup_dont_mark: ....

gasche

I like the final state of the patch and I'm considering merging it (sometime next week?). @garrigue, @alainfrisch, if you have second thoughts about it, now is the time to voice them.

Armael · 2017-11-17T15:41:32Z

I just pushed a batch of small tweaks. These should be rebased into the previous ones before merging, but have not been rebased yet for easy reviewing.

gasche · 2017-11-17T17:40:53Z

The tweaks are good (I just reviewed them) but they don't deserve separate commits. I don't want to squash the whole PRs because the first 4 commits are cleanly separated. Could you squash the tweaks into the relevant commit(s)?

Original design and idea by Arthur Charguéraud

Armael · 2017-11-17T17:45:44Z

Done.

gasche · 2017-11-17T17:48:41Z

Changes


+- GPR#1472: Improve error reporting for missing 'rec' in let-bindings
+  (Arthur Charguéraud and Armaël Guéneau, with help from Gabriel Scherer and
+  Frédéric Bour)


This should be in the "Compiler user-interface and warnings" category, I think.

gasche · 2018-04-09T09:26:43Z

In #1678, @xclerc proposed with an example where the warning is inappropriate:

# let myvar1 = 0;;
val myvar1 : int = 0
# let myvar2 = myvar2 + 1;;
Error: Unbound value myvar2.
Hint: You are probably missing the `rec' keyword on line 1.

@mshinwell reports that there are many wrong hints given when working on the Flambda codebase, and proposes to revert this PR for the time being, which sounds like a reasonable idea.

mshinwell · 2018-04-09T10:03:28Z

(responding to @gasche 's comment on #1678) I don't have any examples to hand unfortunately, but I think they were all typos/mistakes along the lines of what @xclerc wrote above.

damiendoligez · 2018-04-09T14:09:27Z

In preparation for the 4.07 freeze and after discussing with @mshinwell and @Armael, I have reverted this PR (commit 90fbe53), except for commit 3b77d91 (Generalize Env.lookup_* functions to allow disabling marking), which is an independent change and used by a subsequent PR.

A modified version will have to be proposed, see #1678.

This reverts commit c224184, reversing changes made to 2fc77a2. As an exception, commit 3b77d91 (Generalize Env.lookup_* functions to allow disabling marking) is NOT reverted, because it was used by subsequent commits.

gasche reviewed Nov 11, 2017

View reviewed changes

Generalize Val_unbound with a reason

9704f79

Armaël Guéneau added 2 commits November 13, 2017 20:48

Generalize add_pattern_variables to also return pattern variables

11eb5bb

Generalize Env.lookup_* functions to allow disabling marking

3b77d91

Armael force-pushed the improved-error-letrec branch from ae58c1c to fae85a6 Compare November 13, 2017 20:21

gasche approved these changes Nov 15, 2017

View reviewed changes

Armaël Guéneau and others added 2 commits November 17, 2017 18:42

Add specific error messages for missing 'rec' in let bindings

9f75022

Original design and idea by Arthur Charguéraud

Add tests files

a6416ea

Armael force-pushed the improved-error-letrec branch from 149c7af to 804774a Compare November 17, 2017 17:45

Update Changes

0b2d2ce

gasche reviewed Nov 17, 2017

View reviewed changes

Armael force-pushed the improved-error-letrec branch from 804774a to 0b2d2ce Compare November 17, 2017 17:50

gasche merged commit c224184 into ocaml:trunk Nov 17, 2017

Armael mentioned this pull request Dec 11, 2017

"Unbound value X" on recursive function reasonml-community/error-message-improvement#23

Closed

gasche mentioned this pull request Mar 24, 2018

Improved error messages #102

Closed

gasche mentioned this pull request Apr 9, 2018

Do not trigger the "missing rec" hint for simple values #1678

Closed

Armael mentioned this pull request Apr 11, 2018

Missing "rec" hint: v2 #1720

Merged

EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024

[create-pull-request] automated change (ocaml#1472)

07f6897

Conversation

Armael commented Nov 10, 2017

Uh oh!

gasche left a comment

Choose a reason for hiding this comment

Uh oh!

gasche Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

gasche Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

Armael commented Nov 11, 2017

Uh oh!

Armael commented Nov 11, 2017

Uh oh!

gasche commented Nov 11, 2017

Uh oh!

shindere commented Nov 11, 2017 via email

Uh oh!

Armael commented Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Armael commented Nov 11, 2017

Uh oh!

gasche commented Nov 11, 2017

Uh oh!

Armael commented Nov 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Armael commented Nov 13, 2017

Uh oh!

Octachron commented Nov 13, 2017

Uh oh!

Armael commented Nov 13, 2017

Uh oh!

gasche commented Nov 13, 2017

Uh oh!

alainfrisch commented Nov 15, 2017

Uh oh!

Armael commented Nov 15, 2017

Uh oh!

shindere commented Nov 15, 2017 via email

Uh oh!

alainfrisch commented Nov 15, 2017

Uh oh!

gasche left a comment

Choose a reason for hiding this comment

Uh oh!

Armael commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gasche commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Armael commented Nov 17, 2017

Uh oh!

gasche Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

Armael Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

gasche commented Apr 9, 2018

Uh oh!

mshinwell commented Apr 9, 2018

Uh oh!

damiendoligez commented Apr 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Armael commented Nov 11, 2017 •

edited

Loading

Armael commented Nov 13, 2017 •

edited

Loading

Armael commented Nov 17, 2017 •

edited

Loading

gasche commented Nov 17, 2017 •

edited

Loading