Missing "rec" hint: v2 by Armael · Pull Request #1720 · ocaml/ocaml

Armael · 2018-04-11T20:30:57Z

This is a second iteration of #1472, following up on #1678 . Compared to the first iteration, this additionally includes:

The fix to fold_values used by the spellchecking hint, proposed by @xclerc in Do not trigger the "missing rec" hint for simple values #1678;
The reworded version of the hint proposed by @gasche in Do not trigger the "missing rec" hint for simple values #1678, "If this is a recursive definition, you should add...";
Spellchecking as suggested by @gasche (I added a test that demonstrates this);
The proposal by @lpw25 to only enable the hint when the body of the let-bound definitions are syntactic functions. This fixes the "false positives" reported by @xclerc and @mshinwell (I added @xclerc's snippet as a test).

gasche

Looks nice to me.

gasche · 2018-04-11T21:33:09Z

typing/typecore.ml

    if is_recursive then new_env
-    else if not is_recursive then begin
-      (* add ghost bindings to help detecting missing "rec" keywords *)
+    else if not is_recursive && List.for_all sexp_is_fun spat_sexp_list


Do we want List.for_all or List.exists? I would be content with a let .. and .., one of them being a function.

I do not have a strong opinion either way. I guess it should be relatively safe to use List.exists since let .. and .. is rarely used to define something else than recursive functions.

gasche · 2018-04-11T21:33:32Z

testsuite/tests/typing-core-bugs/missing_rec_hint.ml

+  let value2 = value2 (* typo: should be value1 *) + 1 in
+               ^^^^^^
+Error: Unbound value value2
+Hint: Did you mean value1?


At this point in the patch series, there is no heuristic to disable the missing-rec handling for non-functions (added in the next commit 92ff83e ). How come there is no recursive-definition here, does the spellchecking hint override it naturally?

No, it appears I just messed up the tests while rebasing. This should be fixed now.

gasche · 2018-04-12T05:25:00Z

This looks good to merge in trunk to me, but I would be interested in feedback from the flambda people, @xclerc or @mshinwell or @lpw25 for example.

xclerc · 2018-04-12T11:51:46Z

typing/typetexp.ml

+      let (_, line, _) = Location.get_pos_info loc.Location.loc_start in
+      fprintf ppf
+        "@.@[%s %i@]"
+        "Hint: If this is a recursive definition, you should add a 'rec' keyword on line"


This line seems to be a bit long, and the resulting
user-facing message is larger than what is usually
output by the Format module. It might be worthwhile
to both split the line and insert a breakable space.

See also Format.pp_print_text : formatter -> string -> unit which turns all spaces into breakable spaces; here it could be used by turning %s into%a.

(Actually, I am not certain we want to blindly turn all spaces
into breakable ones. Given the shape of the message, there
is a possibility that the break would occur right before the line
number, which is not very nice.)

Well this particular space is not part of the string literal so it actually wouldn't be made breakable.

Also, I am wondering if it would be possible to avoid "@." by opening a vectical box :

fprintf ppf "@[<v>@[Unbound value %a@]" longident lid; spellcheck ppf fold_values env lid; let (_, line, _) = Location.get_pos_info loc.Location.loc_start in fprintf ppf "@,@[%s %i@]@]" "Hint: If this is a recursive definition, you should add a 'rec' keyword on line"

since the behavior of @. tends to mask problems with the formatting.

@gasche sure, but we can get a break before the word "line";
in the surrounding code, breakable spaces are explicitly placed
at strategic points.

gasche · 2018-04-16T20:42:18Z

Another thing we could do is disable the hint if the let-binding is "ghost" code -- automatically generated.

Armael · 2018-04-23T09:39:07Z

I just made the following changes:

the hint message has been split, using a breakable space, as adviced by @xclerc;
the hint is now disabled for ghost code as suggested by @gasche.

Armael · 2018-04-26T12:11:38Z

I just rebased on top of trunk (this was required after @trefis refactoring in #1735).

Any other comments on this?

xclerc · 2018-04-26T13:07:12Z

I just noticed a small inconsistency in the comments:
the comment about the fold_values function in
typing/typetexp.ml here uses "phantom" while
"ghost" is used everywhere.

(I am quite probably the one to blame for introducing
the inconsistency, but cannot edit the PR.)

Armael · 2018-04-26T13:12:49Z

Good catch, thanks.

Armael · 2018-05-05T15:49:28Z

Anything else I can do about this PR?

gasche · 2018-05-05T15:57:19Z

@xclerc, @mshinwell, @lpw25: is there a better way to know if your concerns with the previous version are addressed in this new PR than to merge in trunk and let you see for yourself over time?

xclerc · 2018-05-05T18:06:13Z

Even if the conditions triggering the warning need to be
refined at some point, the current PR looks like a significant
and welcomed improvement. I think it should be merged.

gasche · 2018-05-05T21:35:01Z

@Armael could you move the Changes entry to be in the development version rather than 4.07?

This reverts commit 90fbe53 except for the Changes entry.

Imported from Xavier Clerc patch in ocaml#1678

Armael · 2018-05-06T15:46:59Z

@gasche done and rebased on trunk

Armael force-pushed the improved-error-letrec-v2 branch from 904a970 to b911893 Compare April 11, 2018 20:33

gasche reviewed Apr 11, 2018

View reviewed changes

Armael force-pushed the improved-error-letrec-v2 branch from b911893 to 457da87 Compare April 11, 2018 22:29

xclerc reviewed Apr 12, 2018

View reviewed changes

Armael force-pushed the improved-error-letrec-v2 branch from 831e7a4 to 1e0e6f0 Compare April 23, 2018 09:36

Armael force-pushed the improved-error-letrec-v2 branch 2 times, most recently from 7e8b309 to 3d0f0fa Compare April 26, 2018 12:10

Armael force-pushed the improved-error-letrec-v2 branch from 3d0f0fa to 61528b9 Compare April 26, 2018 13:12

gasche mentioned this pull request Apr 30, 2018

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

Closed

gasche approved these changes May 5, 2018

View reviewed changes

Armael force-pushed the improved-error-letrec-v2 branch from 61528b9 to 465a5f7 Compare May 6, 2018 08:01

Armaël Guéneau added 9 commits May 6, 2018 10:08

Re-implement the hint indicating missing "rec" keywords

41d5658

This reverts commit 90fbe53 except for the Changes entry.

Merge together the several tests for the "missing rec hint"

64d3d1e

Fix fold_values in presence of ghost identifiers

f0dfd2d

Imported from Xavier Clerc patch in ocaml#1678

Allow spellchecking in case of missing rec hint

7f42ee9

Reword the "missing rec hint" to be less assertive

88b2429

Only add ghost bindings when the definition is a syntactic function

dbe2e4c

Add a breakable space to the "missing rec" hint message

d9c4a4e

Minor rewording

eb4dc7f

Only display the "missing rec" hint for non-ghost code

8f936fe

update Changes

acfd378

Armael force-pushed the improved-error-letrec-v2 branch from 465a5f7 to acfd378 Compare May 6, 2018 08:08

gasche merged commit 4a456d3 into ocaml:trunk May 6, 2018

vicuna mentioned this pull request Mar 14, 2019

suite du message #1720 #8178

Closed

Conversation

Armael commented Apr 11, 2018

Uh oh!

gasche left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gasche commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gasche commented Apr 16, 2018

Uh oh!

Armael commented Apr 23, 2018

Uh oh!

Armael commented Apr 26, 2018

Uh oh!

xclerc commented Apr 26, 2018

Uh oh!

Armael commented Apr 26, 2018

Uh oh!

Armael commented May 5, 2018

Uh oh!

gasche commented May 5, 2018

Uh oh!

xclerc commented May 5, 2018

Uh oh!

gasche commented May 5, 2018

Uh oh!

Armael commented May 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gasche commented Apr 12, 2018 •

edited

Loading