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

Eval plugin: mark exceptions #2775

Merged
merged 2 commits into from
Mar 20, 2022
Merged

Conversation

xsebek
Copy link
Contributor

@xsebek xsebek commented Mar 12, 2022

This change will* format exception results in the same way as doctest:

>>> if error "-1" then 1 else 0
------------------
-- current:
-1
------------------
-- new:
*** Exception: -1

*) EDIT: if the configuration option haskell.plugin.eval.config.exception is set to true.

@xsebek
Copy link
Contributor Author

xsebek commented Mar 12, 2022

Hi @jneira, there seems to be no code owner of the eval plugin - do you know who might be familiar with the code? 🙂

Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@pepeiborra pepeiborra added the merge me Label to trigger pull request merge label Mar 12, 2022
Copy link
Member

@Ailrun Ailrun left a comment

Choose a reason for hiding this comment

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

This no "Exception" message is intended AFAIK: https://github.com/haskell/haskell-language-server/tree/master/plugins/hls-eval-plugin#multiline-output

I think we cannot change this at least before we have somewhat "proper" handling of IO.

@Ailrun Ailrun removed the merge me Label to trigger pull request merge label Mar 12, 2022
@Ailrun
Copy link
Member

Ailrun commented Mar 12, 2022

C.C. @tittoassini

@xsebek
Copy link
Contributor Author

xsebek commented Mar 12, 2022

This no "Exception" message is intended AFAIK: https://github.com/haskell/haskell-language-server/tree/master/plugins/hls-eval-plugin#multiline-output

Thanks, @Ailrun, I was not aware of this hack. I do find the missing message a bit confusing, but if someone is using error to emulate putStr then I can let this rest for a while.

@pepeiborra
Copy link
Collaborator

I was not aware of this hack either! We should strive to make this hack unnecessary, some options come to mind:

  • Capturing output can be solved by using the external interpreter (-fexternal-interpreter). We should revisit using it for the eval plugin, but this might be challenging or even impossible.
  • We could wrap all the user code in a custom exception type with a crafted Show instance that doesn't escape output. Then we catch all other exception types and add the "*** Exception" markup.

What do you think @Ailrun ?

@Ailrun
Copy link
Member

Ailrun commented Mar 13, 2022

@pepeiborra I think the second would be more feasible. If we can handle IO better, that would be better, but that may take quite effort

@xsebek
Copy link
Contributor Author

xsebek commented Mar 17, 2022

Hi @pepeiborra, I am not entirely sure what you mean by the second option?

I imagine this could be hacked around by recognizing some prefix like "PRETEND_I_AM_IO: ".

If supporting this hack should involve more effort, I would rather attempt to fix the #1977 instead 🙂

@pepeiborra
Copy link
Collaborator

Sorry I wasn't clear: The only way to properly fix #1977 is by using the external interpreter, which is far from obvious.

@xsebek
Copy link
Contributor Author

xsebek commented Mar 17, 2022

Yes, I understood that the first option is fixing #1977 with an external interpreter though I have no idea how to do that (yet).

What I can not imagine is how to "wrap all the user code" which you suggested as the second option. 🤔

@pepeiborra
Copy link
Collaborator

Yes, I can see now that the 2nd option is not trivial either.

@xsebek
Copy link
Contributor Author

xsebek commented Mar 19, 2022

Another solution that comes to mind would be to make this configurable by the user.

Currently, the eval plugin has the #diff property for WAS/NOW (which is passed to runTests so it's almost there).

So I suggest adding an #exception property (False by default) with the hint:

Enable marking exceptions with *** Exception: similarly to doctest and GHCi.

What do you think, @pepeiborra, @Ailrun? It seems relatively low effort and once we get better IO support, we could just turn it on by default or keep it off. 🙂

@xsebek
Copy link
Contributor Author

xsebek commented Mar 19, 2022

OK, the behaviour is now configurable via haskell.plugin.eval.config.exception (default false) and there is one more test for this configuration option.

Does this resolve your concerns @Ailrun? Btw. thanks for the scaffolding around diff. 🙂

Copy link
Member

@Ailrun Ailrun left a comment

Choose a reason for hiding this comment

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

LGTM :) Here is one minor comment for documentation.

@@ -274,6 +279,8 @@ To display it properly, we can exploit the fact that the output of an error is d
]
```

This assumes you did not turn on exception marking (see #)
Copy link
Member

Choose a reason for hiding this comment

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

It looks like a link is missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I was about to look up how to link sections and got distracted. 😅

Fixed ✔️

@xsebek xsebek force-pushed the eval-exception branch 2 times, most recently from bde1c3e to f92eb82 Compare March 19, 2022 23:56
@xsebek
Copy link
Contributor Author

xsebek commented Mar 20, 2022

At the last minute, I remembered to check the docs for configuration options and added the eval section. 😅

I wonder if it needs to be added to vscode-haskell too?

xsebek added a commit to xsebek/vscode-haskell that referenced this pull request Mar 20, 2022
Eval plugin gains new configuration options in haskell/haskell-language-server#2622 (diff)
and haskell/haskell-language-server#2775 (exception).
Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Docs look fine to me. The way the config options are documented is suboptimal, but this doesn't make it worse!

I don't know what the deal is with exposing options in vscode-haskell...

@pepeiborra pepeiborra enabled auto-merge (squash) March 20, 2022 19:32
@pepeiborra pepeiborra merged commit 1314748 into haskell:master Mar 20, 2022
July541 pushed a commit to July541/haskell-language-server that referenced this pull request Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Eval Plugin: different exception rendering than doctest
4 participants