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

Show code at error location in expect-style tests #1511

Merged
merged 3 commits into from Dec 13, 2017

Conversation

Projects
None yet
3 participants
@gasche
Copy link
Member

gasche commented Dec 3, 2017

Old behavior (lame):

type unknown += Foo;;
(* unknown, not the whole line *)
[%%expect{|
Line _, characters 5-12:
Error: Unbound type constructor unknown
|}];;

new behavior (so awesome):

type unknown += Foo;;
(* unknown, not the whole line *)
[%%expect{|
Line _, characters 5-12:
  type unknown += Foo;;
       ^^^^^^^
Error: Unbound type constructor unknown
|}];;

@gasche gasche changed the title Expect highlight locations Show code at error location in expect-style tests Dec 3, 2017

@@ -640,6 +642,8 @@ module rec Bad : A = Bad;;
module type Alias = sig module N : sig end module M = N end
module F : functor (X : sig end) -> sig type t end
Line _:
module C = Char;;

This comment has been minimized.

@sliquister

sliquister Dec 3, 2017

Contributor

This module C = Char is the first line of the file, because I suppose there is no location and so line is 1 and I am not sure about the characters. It would be preferable to print nothing than to print the first line.

Ideally, the expectation would be clearer about the complete lack of position, so developers are more likely to fix them and reviewers more likely to point them out.

This comment has been minimized.

@sliquister

sliquister Dec 4, 2017

Contributor

Oh, the characters are probably 0-0, which is why nothing is underlined.

This comment has been minimized.

@gasche

gasche Dec 4, 2017

Author Member

Actually, I think that grepping for Line _: is enough to detect fishy locations: the characters are not printed when they look non-sensical, and I think this coincides with what you want to detect. In that case, I would suggest not changing anything in the PR specifically: (1) it's an orthogonal concern and (2) the reasonable need that you suggested can already be satisfied in the current state (if my guess is right that Line _: coincides with the situation where you think we should "be clearer").

This comment has been minimized.

@sliquister

sliquister Dec 4, 2017

Contributor

Oh ok, this is the one place where something fishy is going on.
It's actually more fishy than I first thought, because ocaml/expect_test give one error, and ocamlc/ocamlopt give a different looking error, which I think is a better version of the same error, with a position in particular.

In any case, it still seems to me that the whole show_code_at_location call should be below a if startchar >= 0, to avoid printing unrelated code.

This comment has been minimized.

@gasche

gasche Dec 4, 2017

Author Member

expect_test is using Toploop which is the ocaml (toplevel) path, maybe the discrepancy can be reproduced there? You are right that it's interesting enough to investigate, I'll try to have a look.

| Some lexbuf ->
Location.show_code_at_location ppf lexbuf loc
end;
()

This comment has been minimized.

@sliquister

sliquister Dec 3, 2017

Contributor

Wouldn't it be plain simpler to call the existing highlight_dumb function without the ~print_chars flag, which would change the output from:

Line _, characters 1-2:
  text
  ^^

to:

Characters 1-2:
  text
  ^^

This comment has been minimized.

@gasche

gasche Dec 4, 2017

Author Member

So we tried that first, and the result is something like

Characters 346-373:

and it is not stable to re-running the script again with -principal filled in, so it doesn't actually work.

This comment has been minimized.

@sliquister

sliquister Dec 4, 2017

Contributor

Ok, expect_tests works on the whole file, so positions are from the beginning of the file, whereas the toplevel works phrase by phrase. Fair enough.

@sliquister

This comment has been minimized.

Copy link
Contributor

sliquister commented Dec 3, 2017

Presumably the goal is to be able to review the location of errors more effectively? Looks good to me.

You apparently missed testsuite/tests/typing-misc/pr6939.ml-{flat,noflat} in your rewrite.

@gasche

This comment has been minimized.

Copy link
Member Author

gasche commented Dec 4, 2017

Presumably the goal is to be able to review the location of errors more effectively? Looks good to me.

Yes, thanks!

You apparently missed testsuite/tests/typing-misc/pr6939.ml-{flat,noflat} in your rewrite.

Indeed. I fixed the -flat one and rebased, but -noflat seems to be actually failing in trunk, I'll have to investigate more -- see #1471 (comment).

@gasche

This comment has been minimized.

Copy link
Member Author

gasche commented Dec 4, 2017

@sliquister have you reviewed the whole PR in enough details that you would formally "Approve" of it?

@sliquister
Copy link
Contributor

sliquister left a comment

I didn't look at every single test in details, but what I looked at was good. I think you should make the one line change to avoid printing code when the position has negative characters before merging, though.

@gasche gasche force-pushed the gasche:expect-highlight-locations branch from 49f2d41 to 5ee39bf Dec 10, 2017

@gasche

This comment has been minimized.

Copy link
Member Author

gasche commented Dec 10, 2017

I just performed the change requested by @sliquister. Thanks!

@damiendoligez
Copy link
Member

damiendoligez left a comment

I have reviewed the compiler changes and they look good. I didn't look at all the tests but I don't think they need to be carefully reviewed.

Not merged because @gasche might want to investigate the appveyor failure.

@gasche

This comment has been minimized.

Copy link
Member Author

gasche commented Dec 12, 2017

@damiendoligez thanks a lot for the additional review.

I cannot make sense of the AppVeyor failure: the log appears to be cut (both in the web view and in the raw log) in the middle of compilation, it hasn't reached the testsuite yet. @dra27, would you by chance have an idea of what is happening here?

(Is there a way to restart an AppVeyor build? I could close-reopen the issue, but that would also restart the Travis build.)

I will have to fix the conflicts in any case, so I will push again, which may make the issue disappear.

gasche added some commits Dec 3, 2017

Location.show_code_at_location: highlight the code at a given location
This uses the "highligh_dumb" machinery, but is explicitly designed to
not print the location itself. This is for use in the expect-style
tests; the expect-test modification will come in a separate commit,
but it looks as nice as the following:

[%%expect{|
Line _, characters 1-4:
 #bar) -> ();;
  ^^^
Error: Unbound class bar
|}];;

@gasche gasche force-pushed the gasche:expect-highlight-locations branch from 5ee39bf to a607486 Dec 12, 2017

@gasche gasche merged commit a923eb4 into ocaml:trunk Dec 13, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.