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

Attach explanation message to diagnostic message #16787

Merged
merged 1 commit into from
Feb 3, 2023

Conversation

lwronski
Copy link
Contributor

When the -explain flag was turned on, an explanation message wasn't displayed in the Metals and Scala CLI. This was because both of these tools only print the message from the Problem.message field.

To resolve this issue, I attached the explanation content to the Problem.message field, which will now be displayed in the Metals and Scala CLI.

This PR addresses a issue from scala-cli.

@julienrf
Copy link
Contributor

In the long run, should we pass the explanation to another field of Problem (just like diagnosticCode is a separate field), instead of concatenating it to the message?

@lwronski lwronski marked this pull request as draft January 30, 2023 17:14
@prolativ
Copy link
Contributor

diagnosticCode should be just what the name says - a code/number of an error. Actually there doesn't currently seem to be any more suitable field in a Diagnostic to carry explanations of messages (note that explanations are not fixed for a given error number but their content can change depending on the actual code that caused the error and what the compiler manages to diagnose). Adding some new field to Diagnostic would require changes in LSP, but this doesn't really seem worth the effort as that would only allow us to display explanations in some fancy way but it's actually the content of the explanation message that matters for users

@lwronski lwronski marked this pull request as ready for review January 30, 2023 21:48
@julienrf
Copy link
Contributor

julienrf commented Jan 31, 2023

Adding some new field to Diagnostic would require changes in LSP,

This is indeed what I meant.

but this doesn't really seem worth the effort as that would only allow us to display explanations in some fancy way but it's actually the content of the explanation message that matters for users

OK, good to know. I thought it could be useful to allow UIs to adapt the amount of information shown based on the available space. For instance, show first the message, but if there is an additional explanation then show a button “more details” that expands and shows the full explanation.

But in practice I don’t really know if those explanations are really verbose or not.

Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

I'd actually recommend not merging this unless I'm misunderstanding.

Regarding usage in scala-cli, wouldn't it be more appropriate to instead of relying on the message to rely on rendered? This is what sbt does for example. By adding the explanation to the message we're essentially making rendered useless aren't we? I don't think we should do this. From my understanding this exact usecase is why rendered was introduced.

On another note, @lwronski do you have a screenshot on what this looks like in VS Code? In some editors these errors will be gigantic and probably won't display that nicely. I think before we merge this we should ensure that it actually looks nice in the various supported editors. If i'm not mistaken that's why we didn't do this earlier. I'd also be curious to see how other editors that support LSP will display this. We don't want to introduce this and mutilate the way errors look in a handful of editors, even if it's for a minority of users.

I think if we wanted to carry this information forward a more appropriate way would be use use the explanation field in the DiagnosticCode. This would allow for that information to be structurally forwarded in a way that a client could choose how to display it. I'm not 100% sure without looking more deeper in dotty, but ideally the message should be a minimal message whereas the explanation should be the additional information associated with the diagnostic. Right now I think there might be overlap as the message is also contained in the explanation here. So ideally that wouldn't be the case.

@prolativ
Copy link
Contributor

Is there any formal requirement on a message to be 'short'? Is there any limit on the length? Theoretically some kinds of errors might produce relatively long outputs even without -explain enabled. As far as I know the purpose of this flag was actually not limiting the size of error messages but avoiding slowdowns in compilation is a general case when more detailed information about an error was not requested.

Regarding explanation in DiagnosticCode (defined in xsbti interfaces) I assume it should correspond to codeDescription in Diagnostic in LSP. Unfortunately CodeDescription specifies only one field href: URI, assuming the explanation to be served externally and fixed for a given type of error. However the explanation added by -explain varies depending on the actual source code being compiled.

IMO maybe explanation field inside data could be an option but data is there only LSP at the moment

@ckipp01
Copy link
Member

ckipp01 commented Jan 31, 2023

Is there any formal requirement on a message to be 'short'? Is there any limit on the length?

I don't think so, but since using explain has a lot of "structured" output in the string we should probably make sure it looks alright in the actual editor since it will make many of them longer.

As far as I know the purpose of this flag was actually not limiting the size of error messages but avoiding slowdowns in compilation is a general case when more detailed information about an error was not requested.

Gotcha, that makes sense. However with this approach then what ends up being the difference of rendered and message?

Unfortunately CodeDescription specifies only one field href: URI, assuming the explanation to be served externally and fixed for a given type of error.

Ahhh, I didn't even realize it was only a URI! That's sort of a bummer but I guess it makes sense. All the more reason to have a stable URI for our diagnostic codes that someone can reference.

IMO maybe explanation field inside data could be an option but data is there only LSP at the moment

Yea, I'm not fully sold. Mainly because in LSP anyways it says:

A data entry field that is preserved between a textDocument/publishDiagnostics notification and textDocument/codeAction request.

Which doesn't really seem to apply here or would break assumptions about what it's meant to be used for.

I guess if inside of message is the only place to put it, sure, but then the question about the differentiation of rendered still stands. Plus we should see what it looks like in various editors as well.

@prolativ
Copy link
Contributor

prolativ commented Feb 1, 2023

I don't think so, but since using explain has a lot of "structured" output in the string we should probably make sure it looks alright in the actual editor since it will make many of them longer.

Explanations can have various shapes but even if we keep them separate from basic messages, I wouldn't say they have much structure that we could handle in a special way (at least not more than basic messages generated without -explain)

Gotcha, that makes sense. However with this approach then what ends up being the difference of rendered and message?

It seems that rendered is mainly intended to be used in a REPL session or when compiling code from the command line. Rendering adds some fancy ways of displaying messages when all the information about an error have to be shown in a single piece of text, e.g. it reprints erroneous lines of code with underlined positions and wraps extra messages in ASCII frames just as shown here VirtusLab/scala-cli#1285

In general our main problem is that extra messages from -explain are entirely lost now when code gets compiled via BSP, which should be considered a bug. Of course we might later try to improve the way how things get shown in IDEs, but that would be the next step with lower priority

@ckipp01
Copy link
Member

ckipp01 commented Feb 1, 2023

Rendering adds some fancy ways of displaying messages when all the information about an error have to be shown in a single piece of text

But isn't this what we're trying to do with including the -explain output with the message?

In general our main problem is that extra messages from -explain are entirely lost now when code gets compiled via BSP, which should be considered a bug.

Totally, agreed.

@prolativ
Copy link
Contributor

prolativ commented Feb 1, 2023

Rendering adds some fancy ways of displaying messages when all the information about an error have to be shown in a single piece of text

But isn't this what we're trying to do with including the -explain output with the message?

I would say the compiler and tooling have slightly different understanding of explanation. And rendered seems to be the entire Problem/Diagnostic displayed in a fully textual form suitable for presenting in a console

@lwronski
Copy link
Contributor Author

lwronski commented Feb 1, 2023

IMO maybe explanation field inside data could be an option but data is there only LSP at the moment

@prolativ FYI - The data field is also available in the BSP. It was added some time ago in this PR https://github.com/build-server-protocol/build-server-protocol/pull/363/files.

@prolativ
Copy link
Contributor

prolativ commented Feb 1, 2023

@lwronski good to know. However we still need to decide what structure data should have when returned from the compiler. I think putting the explanation directly in a message still the easiest to reach and the most profitable solution for us at the moment. Should we merge this PR then?

@ckipp01
Copy link
Member

ckipp01 commented Feb 1, 2023

@prolativ FYI - The data field is also available in the BSP. It was added some time ago in this PR https://github.com/build-server-protocol/build-server-protocol/pull/363/files.

True, but again I think we want to be careful what we stick in here since this data field will be forwarded all the way to the LSP client and we don't want to have to be storing a bunch of stuff client side that might not have anything to do with the original purpose, code actions.

@prolativ prolativ merged commit dd92c85 into scala:main Feb 3, 2023
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.

4 participants