Improve model-building error messages - #1109
Conversation
|
👋 Welcome back asotona! A progress list of the required criteria for merging this PR into |
|
@asotona This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
PaulSandoz
left a comment
There was a problem hiding this comment.
Very good, something I think we can iterate on. Two things:
- I wonder if we should have an internal option in
OpWriterto enable the modified behavior, otherwise it can behave strangely. And it seems useful to consolidate catching of exceptions within some general lambda accepting method. - I think we need to be very consistent in reusing the terminology in the API documentation.
| assertEquals(""" | ||
| Block has no terminating operation as the last operation | ||
| ^block_0: | ||
| ^~~~~~~~ missing terminal op |
There was a problem hiding this comment.
| ^~~~~~~~ missing terminal op | |
| ^~~~~~~~ missing terminating op |
| ^block_0: | ||
| ^~~~~~~~ missing terminal op | ||
| %0 : java.type:"int" = constant @1; | ||
| """, assertThrows(IllegalStateException.class, () -> func("f", body)).getMessage()); |
There was a problem hiding this comment.
Can you place the lambda on the next line? as it makes it easier to scan
| assertEquals(""" | ||
| A new operation cannot directly use a value from a completed code model | ||
| ^block_0(%0 : java.type:"int"): | ||
| ^~ value from completed model |
There was a problem hiding this comment.
Can we reuse the same terminology in the error message? i.e. the value's declaring block is built, since "completed model" is not a phrase we use. This applies in other cases too when a block is unobservable (since it is still being built).
| assertEquals(""" | ||
| Body of operation is connected to a different ancestor body | ||
| ^block_1: | ||
| ^~~~~~~~ operation body |
There was a problem hiding this comment.
Are we referring to the operation's ancestor body or its parent block?
This PR improves model-building error messages and provides additional diagnostic information, including model fragments. It also enables incomplete models to be printed.
Example of the improved error message:
See
TestDiagnosticsfor more.Progress
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/babylon.git pull/1109/head:pull/1109$ git checkout pull/1109Update a local copy of the PR:
$ git checkout pull/1109$ git pull https://git.openjdk.org/babylon.git pull/1109/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1109View PR using the GUI difftool:
$ git pr show -t 1109Using diff file
Download this PR as a diff file:
https://git.openjdk.org/babylon/pull/1109.diff
Using Webrev
Link to Webrev Comment