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.

Sign up for GitHub

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

Jump to bottom

fix: include all diagnostics #6940

Merged
merged 1 commit into from
Feb 8, 2023
Merged

fix: include all diagnostics #6940

merged 1 commit into from
Feb 8, 2023

Conversation

rgrinberg
Copy link
Member

@rgrinberg rgrinberg commented Jan 26, 2023 •
edited
Loading

Some compilation commands emit more than one diagnostic. For example,
ocamlc can emit more than one deprecation or unused error warning.

The previous behavior would be to just take the first error and drop the
others. This PR fixes the behavior to include all errors extracted out
of a command.

cc @ddickstein

@rgrinberg rgrinberg force-pushed the ps/rr/fix__include_all_diagnostics branch from 343360c to c927c86 Compare January 26, 2023 17:56
@rgrinberg rgrinberg requested a review from snowleopard January 26, 2023 17:57
@rgrinberg rgrinberg added this to the 3.7.0 milestone Jan 26, 2023
@rgrinberg rgrinberg force-pushed the ps/rr/fix__include_all_diagnostics branch 2 times, most recently from 83d4424 to 4d9507a Compare January 26, 2023 18:00
@ddickstein
Copy link
Contributor

Very exciting! I think my main question pertains not to the PR but the code it's built on - dune/otherlibs/ocamlc_loc/src/ocamlc_loc.ml. How does it distinguish one error from the next? Would it just be wherever lines that can be parsed as locations appear, e.g., File "foo.ml", lines 8-10, characters 0-3:? If so, does this only apply to errors generated by tools outside the compiler? Because some compiler errors have these lines embedded in them, such as value or type mismatches between foo.ml and foo.mli.

@rgrinberg
Copy link
Member Author

How does it distinguish one error from the next? Would it just be wherever lines that can be parsed as locations appear, e.g., File "foo.ml", lines 8-10, characters 0-3:?

Yes, that's basically how it works.

Because some compiler errors have these lines embedded in them, such as value or type mismatches between foo.ml and foo.mli.

There's special handling for them. These are called "related" errors in the rpc API. We detect them by parsing locations that start with white spaces rather than toplevel errors which are preceded with new line characters.

@rgrinberg
Copy link
Member Author

@snowleopard do you have time to take a look at this? would be cool to have this for 3.7

@snowleopard
Copy link
Collaborator

snowleopard commented Feb 1, 2023 •
edited
Loading

@snowleopard do you have time to take a look at this? would be cool to have this for 3.7

I wonder if I'm the best person to review this code. @ddickstein would like to review it instead since you're thinking about Dune RPC a lot more than me these days?

@ddickstein
Copy link
Contributor

ddickstein commented Feb 1, 2023 via email

@snowleopard
Copy link
Collaborator

Thanks!

@emillon
Copy link
Collaborator

emillon commented Feb 8, 2023

@ddickstein would you be able to review this PR this week? 3.7 is currently blocked on that, so we can move it back to 3.8 if that's not possible. Thanks!

ddickstein
ddickstein suggested changes Feb 8, 2023
View reviewed changes
src/dune_engine/compound_user_error.ml Outdated
@@ -1,7 +1,7 @@
open Import

module T : sig
type t = private
type t =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a bit strange that Compound_user_error.t can potentially be a single user error. How about naming this User_errors.t? Also, unless there is good reason to think that in the case of multiple errors the first one is especially meaningful, I would just call the fields hd and tl. Or, introduce a generic Nonempty_list.t type in dune if it doesn't already exist, and then make User_errors.t a User_message.t Nonempty_list.t. This can be resolved in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, there might be some confusion here. A Compound_user_error.t is always a single error. The related field is just for additional locations the error might have.

Also, unless there is good reason to think that in the case of multiple errors the first one is especially meaningful, I would just call the fields hd and tl. Or, introduce a generic Nonempty_list.t type in dune if it doesn't already exist, and then make User_errors.t a User_message.t Nonempty_list.t. This can be resolved in a separate PR.

That would be possible. But note that there are some differences between main and related that aren't reflected adequately in the types. For example, every single "related" message must have a location. While the main message may omit it. Should I perhaps improve the type of related here instead?

src/dune_engine/build_system.ml
{ id : Id.t
; exn : Exn_with_backtrace.t
}
| Diagnostic of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be Diagnostics? Same question for the field name.

Copy link
Member Author

@rgrinberg rgrinberg Feb 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, a diagnostic here is a single logical error with possibly multiple "related" locations. Same as in LSP.

test/expect-tests/dune_rpc_e2e/dune_rpc_diagnostics.ml
@@ -711,5 +711,86 @@ let g = A.f
; [ "related"; [] ]
; [ "targets"; [] ]
]
]
[ "Add"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test output is surprising me. Based on the code, I would have expected all of these new entries to appear in the "related" list of the diagnostic above. Why are they appearing independently with empty "related" lists of their own?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because they are separate errors. "related" errors are really for errors that have more than one location. The most common example is an .ml/.mli mismatch where there's a location in the .ml and the .mli inside the error message.

@rgrinberg rgrinberg force-pushed the ps/rr/fix__include_all_diagnostics branch 2 times, most recently from 00eb155 to a7c6629 Compare February 8, 2023 19:39
@rgrinberg
fix: include all diagnostics
966f43a 
Some compilation commands emit more than one diagnostics. For example,
ocamlc can emit more than one deprecation or unused error warning.

The previous behavior would be to just take the first error and drop the
others. This PR fixes the behavior to include all errors extracted out
of a command.

Signed-off-by: Rudi Grinberg <[email protected]>

<!-- ps-id: 2ecf33bf-dbff-475b-b620-5c182698c8df -->
@rgrinberg rgrinberg force-pushed the ps/rr/fix__include_all_diagnostics branch from a7c6629 to 966f43a Compare February 8, 2023 20:03
@ddickstein
Copy link
Contributor

Thanks for clarifying - I was misunderstanding some of what this was doing. I understand it better now and the code looks good to me. 👍

@rgrinberg rgrinberg merged commit 873c6ba into main Feb 8, 2023
@rgrinberg rgrinberg deleted the ps/rr/fix__include_all_diagnostics branch February 8, 2023 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ddickstein ddickstein ddickstein approved these changes

@snowleopard snowleopard Awaiting requested review from snowleopard

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.7.0
Development

Successfully merging this pull request may close these issues.
4 participants
@rgrinberg @ddickstein @snowleopard @emillon