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

Missing "rec" hint: v2 #1720

Merged
merged 10 commits into from May 6, 2018

Conversation

Projects
None yet
4 participants
@Armael
Copy link
Member

commented Apr 11, 2018

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 #1678;
  • The reworded version of the hint proposed by @gasche in #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).

@Armael Armael force-pushed the Armael:improved-error-letrec-v2 branch from 904a970 to b911893 Apr 11, 2018

@gasche
Copy link
Member

left a comment

Looks nice to me.

let exp_env =
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

This comment has been minimized.

Copy link
@gasche

gasche Apr 11, 2018

Member

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

This comment has been minimized.

Copy link
@Armael

Armael Apr 11, 2018

Author Member

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.

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

This comment has been minimized.

Copy link
@gasche

gasche Apr 11, 2018

Member

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?

This comment has been minimized.

Copy link
@Armael

Armael Apr 11, 2018

Author Member

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

@Armael Armael force-pushed the Armael:improved-error-letrec-v2 branch from b911893 to 457da87 Apr 11, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented Apr 12, 2018

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.

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 comment has been minimized.

Copy link
@xclerc

xclerc Apr 12, 2018

Contributor

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.

This comment has been minimized.

Copy link
@gasche

gasche Apr 12, 2018

Member

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.

This comment has been minimized.

Copy link
@xclerc

xclerc Apr 12, 2018

Contributor

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

This comment has been minimized.

Copy link
@gasche

gasche Apr 12, 2018

Member

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

This comment has been minimized.

Copy link
@Octachron

Octachron Apr 12, 2018

Contributor

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.

This comment has been minimized.

Copy link
@xclerc

xclerc Apr 12, 2018

Contributor

@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

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

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

@Armael Armael force-pushed the Armael:improved-error-letrec-v2 branch from 831e7a4 to 1e0e6f0 Apr 23, 2018

@Armael

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2018

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 Armael force-pushed the Armael:improved-error-letrec-v2 branch 2 times, most recently from 7e8b309 to 3d0f0fa Apr 23, 2018

@Armael

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2018

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

Any other comments on this?

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2018

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 Armael force-pushed the Armael:improved-error-letrec-v2 branch from 3d0f0fa to 61528b9 Apr 26, 2018

@Armael

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2018

Good catch, thanks.

@Armael

This comment has been minimized.

Copy link
Member Author

commented May 5, 2018

Anything else I can do about this PR?

@gasche

This comment has been minimized.

Copy link
Member

commented May 5, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented May 5, 2018

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

This comment has been minimized.

Copy link
Member

commented May 5, 2018

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

@gasche

gasche approved these changes May 5, 2018

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

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

@Armael

This comment has been minimized.

Copy link
Member Author

commented May 6, 2018

@gasche done and rebased on trunk

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vicuna vicuna referenced this pull request Mar 14, 2019

Closed

suite du message #1720 #8178

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.