Skip to content

Add a delegate to errors.d to make error handling configurable#10072

Merged
RazvanN7 merged 3 commits intodlang:masterfrom
ManishKhurana11:lspErrorDiagnostic
Jan 6, 2020
Merged

Add a delegate to errors.d to make error handling configurable#10072
RazvanN7 merged 3 commits intodlang:masterfrom
ManishKhurana11:lspErrorDiagnostic

Conversation

@ManishKhurana11
Copy link
Contributor

@ManishKhurana11 ManishKhurana11 commented Jun 22, 2019

This PR adds a delegate to errors.d to make error handling configurable

@dlang-bot
Copy link
Contributor

dlang-bot commented Jun 22, 2019

Thanks for your pull request and interest in making D better, @ManishKhurana11! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#10072"

Copy link
Contributor

@jacob-carlborg jacob-carlborg left a comment

Choose a reason for hiding this comment

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

I've been planning to do something similar. I don't think this is the best approach. I've been discussing this with Walter and he prefers we add a delegate to make the error handling configurable. By default the delegate would do what verrorPrint does today.

@WalterBright
Copy link
Member

LSP Diagnostic request

What is that?

@JinShil
Copy link
Contributor

JinShil commented Jun 22, 2019

LSP

That appears to be the Language Server Protocol GSoC project: https://forum.dlang.org/post/tatciprksgbbnczqfxbc@forum.dlang.org

@WalterBright
Copy link
Member

That appears to be

We shouldn't have to guess.

Besides, @jacob-carlborg is right. Instead of adding more and more data structures and complexity for error reporting in the compiler, just add a sink delegate that the compiler sends the message to, and the recipient then does what it needs.

@jacob-carlborg
Copy link
Contributor

LSP

For reference, LSP (Language Server Protocol) [1] is a client-server protocol for implementing intellisense (code completion, go to definition, and so on). The client would be a text editor or an IDE and the server would be a daemon running in the background containing all the logic for the intellisense. The idea is that one doesn't have to implement the same intellisense logic (for a given language) for all the text editors out there. It's enough to implement once. Same idea as the separation of a compiler frontend and backend.

Manish Khurana is a Google Summer of Code student that is working on an LSP server that is using DMD as a library.

[1] https://langserver.org

@ManishKhurana11 ManishKhurana11 changed the title Add struct ErrorRecord in globals.d and it's usage in other files Add a delegate to errors.d to make error handling configurable Jun 24, 2019
@jacob-carlborg
Copy link
Contributor

Please add a new parameter to the initDMD function [1] that sets the diagnostic handler.

[1]

dmd/src/dmd/frontend.d

Lines 105 to 108 in bc6bb99

void initDMD(
const string[] versionIdentifiers = [],
ContractChecks contractChecks = ContractChecks()
)

@ManishKhurana11 ManishKhurana11 force-pushed the lspErrorDiagnostic branch 2 times, most recently from 0530b1d to 6878910 Compare June 27, 2019 11:31
@rainers
Copy link
Member

rainers commented Jun 28, 2019

I think this is fine, but please add a test similar to the ones in https://github.com/dlang/dmd/blob/master/test/unit

@ManishKhurana11 ManishKhurana11 force-pushed the lspErrorDiagnostic branch 2 times, most recently from 1950037 to 0c17d1e Compare June 28, 2019 18:42
@rainers
Copy link
Member

rainers commented Aug 10, 2019

I've pushed some minor changes:

  • added bool return value to the diagnostic handler to allow continuing with standard error output
  • made the handler variable __gshared as TLS is not used in dmd

If this is merged, the DiagnosticReporter classes could build on it and could be moved from the core compiler to the frontend library as in master...rainers:move_diagreport. @jacob-carlborg would you be ok with this?

@rainers rainers force-pushed the lspErrorDiagnostic branch from af15bfa to f5711ae Compare August 10, 2019 15:40
@jacob-carlborg
Copy link
Contributor

Yes.

@rainers rainers force-pushed the lspErrorDiagnostic branch from f5711ae to 1356f3b Compare August 10, 2019 18:35
@RazvanN7
Copy link
Contributor

What is the status of this? Should we merge this or do you have an alternate solution @jacob-carlborg ?

@jacob-carlborg
Copy link
Contributor

I don’t have an alternate solution. But the handler should be set to null in deinitializeDMD or somewhere.

@rainers
Copy link
Member

rainers commented Dec 20, 2019

But the handler should be set to null in deinitializeDMD or somewhere.

Ok, I added that.

@rainers rainers force-pushed the lspErrorDiagnostic branch from bd158ce to 0f40610 Compare January 4, 2020 11:20
@rainers
Copy link
Member

rainers commented Jan 4, 2020

@jacob-carlborg Is your change request resolved?

As I have contributed to this PR I'm biased and hesitate to merge it myself, I'm using it in my own fork for a language server, though, and would like to reduce differences...

@jacob-carlborg
Copy link
Contributor

Is your change request resolved?

Yes.

@RazvanN7 RazvanN7 merged commit 7cf4446 into dlang:master Jan 6, 2020
@rainers
Copy link
Member

rainers commented Jan 6, 2020

Thanks!

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.

8 participants