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

Location: generalize [highlight_dumb] to handle several locations #1894

Merged
merged 2 commits into from Jul 12, 2018

Conversation

Projects
None yet
2 participants
@Armael
Copy link
Member

commented Jul 10, 2018

The purpose of this PR is to generalize highlight_dumb to make it consistent with highlight_terminfo, that is, able to highlight several location at the same time.

highlight_terminfo (which highlights the error by underlining it using ANSI terminal features) supports highlighting several locations simultaneously. Note that this feature is at the moment only used in one error -- "unmatched parenthesis", which highlights both the erroneous token and the unmatched parenthesis.

Since this feature might be useful more generally in the future (for the future parsing error messages maybe?), it seems useful to clean-up the current situation.

Before this patch

highlight_dumb only supports highlighting a single location, and is passed List.hd of the locations (in highlight_locations):

with an ANSI terminal (where __ denotes underlined text):

# let x = __(__1 + 2 __in__ ();;
Syntax error: ')' expected, the highlighted '(' might be unmatched

with TERM=dumb, only the open parenthesis gets highlighted:

# let x = (1 + 2 in ();;                                                                                  
Characters 8-9:
  let x = (1 + 2 in ();;
          ^
Syntax error: ')' expected, the highlighted '(' might be unmatched

After this patch

highlight_dumb highlights all locations:

# let x = (1 + 2 in ();;
Characters 8-17:
  let x = (1 + 2 in ();;
          ^      ^^
  Syntax error: ')' expected, the highlighted '(' might be unmatched

If the locations occur or span across several lines, the "multi-line" style for displaying errors is used:

# let x = (1
  +
    2 in ();;
Characters 8-19:
  ........(.
  .
  ....in.....
  Syntax error: ')' expected, the highlighted '(' might be unmatched
@gasche
Copy link
Member

left a comment

This is a nice refactoring, but I still have minor questions about small changes in the code. I would be happy to approve once those minor questions are resolved.

Do we have test for this code, and you didn't need to change them because they behave exactly as before, or do we not test it? (In that case...)

There is a small refactoring that I think could make the final code even more readable -- at the cost of being farther away from the original. I'm giving the idea below in case you are interested, but I don't think this is necessary.

Right now it is a bit irritating that a single loop has to handle both the single-line and multi-line cases, which do not share much logic: it looks like moving the distinction out of the loop would help readability, but right now we cannot because of the incr line business which is intertwined with the printing code.

In the loop where you compute line_start, line_end, you could also compute the start and end position of those lines. Then I have the impression that you could separate the single-line and multi-line situations right after that, and you wouldn't even need to maintain the line reference within any of the two loops.

| '\n' ->
if !line = !line_start && !line = !line_end then begin
(* loc is on one line: underline location *)
Format.fprintf ppf "@, ";

This comment has been minimized.

Copy link
@gasche

gasche Jul 11, 2018

Member

This underline-location start format is "@, ", but your diff replaces it with "@," only. Why?

end;
if !line >= !line_start && !line <= !line_end then begin
Format.fprintf ppf "@,";
if pos < loc.loc_end.pos_cnum then Format.pp_print_string ppf " "

This comment has been minimized.

Copy link
@gasche

gasche Jul 11, 2018

Member

Any idea what this test was for? Am I correct that you removed it completely?

Format.pp_print_char ppf c
done;
Format.fprintf ppf "@]"
else begin

This comment has been minimized.

Copy link
@gasche

gasche Jul 11, 2018

Member

I think the code would be more readable with just else if here, and a begin..end after the then instead.

@Armael Armael force-pushed the Armael:location-dumb-refactor2 branch 2 times, most recently from ea00c53 to 33f3508 Jul 11, 2018

@Armael

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2018

Thanks for the review. There were some bugs related to indentation, I just fixed them and the tests should now pass.

  • For your questions asking why I removed the printing of " " in some places: the code got refactored to use a Format box to handle indenting everything by two spaces, instead of manually printing two spaces at the beginning of each line.

  • Wrt tests: the code behaves like before except for multi-locations errors in dumb mode. This apparently wasn't tested, so I could add a test indeed. Any idea where this test should go? (in which directory)

  • I agree having a single loop to handle both cases is a bit incomprehensible. In a first patch I implemented your suggestion, but wasn't convinced. I will try again.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

(Again, the suggestion is not at all blocking for the PR.)

Where to put the tests:

cd testsuite/test; git grep ocamlcommon
ast-invariants/test.ml:   include ocamlcommon
parsetree/test.ml:   include ocamlcommon
ppx-contexts/test.ml:include ocamlcommon
self-contained-toplevel/main.ml:*** ocaml with ocamlcommon
tool-toplevel/pr7751.ml:   include ocamlcommon

I think you could open an internals or compilerlibs directory for internal unit tests, otherwise I would go for tool-toplevel by affinity.

@Armael

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2018

@gasche I implemented your suggestion (in a separate commit, tell me if you want it squashed with the previous one) and added tests.

@Armael Armael force-pushed the Armael:location-dumb-refactor2 branch from ccb7980 to 086d59d Jul 11, 2018

@Armael

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2018

Also I don't think a Changes entry is needed.

@Armael

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2018

One test is failing because the new code does not have the same behavior than the old code when given Location.none, which seems to happen with the following toplevel phrase:

type 'a t = private < x : int; .. > as 'a;;

This looks like a bug to me, but I'll also try to fix this patch to get the same behavior as before.

@Armael

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2018

Actually the existing behavior is somewhat nonsensical (the first line of the input gets printed without anything highlighted). I modified the patch to instead explicitly filter out dummy locations. If no locations are provided or if all locations are dummy, then the function is a no-op. I updated the failing test accordingly.

) locs
(* Ignore dummy locations *)
|> List.filter (fun (s, e) -> not (s = -1 && e = -1))
in

This comment has been minimized.

Copy link
@gasche

gasche Jul 11, 2018

Member

Fun fact: this is Misc.Stdlib.List.filter_map.

@gasche

gasche approved these changes Jul 11, 2018

Copy link
Member

left a comment

Looks good to me.

I think you can squash indeed if you like -- the total diff makes sense as an atomic change.

I will merge when the CI passes.

(Don't take my filter_map remark too seriously, I don't think it would make the code more readable.)

Do you like the new separate-passes code better?

@Armael Armael force-pushed the Armael:location-dumb-refactor2 branch 2 times, most recently from ef2c2ab to 2695d6b Jul 11, 2018

@Armael

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2018

(Don't take my filter_map remark too seriously, I don't think it would make the code more readable.)

Too late!

Do you like the new separate-passes code better?

Yes, I think it's better.

I just squashed the commits.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

A Changes entry would be nice.

@Armael Armael force-pushed the Armael:location-dumb-refactor2 branch from 2695d6b to 6503688 Jul 12, 2018

@Armael

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2018

Changes entry added

@gasche gasche merged commit 0788bda into ocaml:trunk Jul 12, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Member

commented Jul 12, 2018

Thanks!

(In Changes, it is better to have the credit parenthesis on its own line.)

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.