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

fix: Diagnostics lost during JSON serialization #170

Conversation

slavek-kucera
Copy link
Contributor

No description provided.

@slavek-kucera slavek-kucera added the bug Something isn't working label Aug 20, 2021
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@@ -64,7 +112,7 @@ std::pair<semantics::operands_si, semantics::remarks_si> statement_fields_parser

diagnostic_consumer_transform add_diag_subst([&field, &add_diag, after_substitution](diagnostic_op diag) {
if (after_substitution)
diag.message = "While evaluating the result of substitution '" + field + "' => " + std::move(diag.message);
diag.message = decorate_message(field, diag.message);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to run the sanitizer in the language_server to sanitize all diagnostics? I think there are some more diagnostics that show names of symbols... Although those are probably guaranteed to be 'nice' strings since they were lexed as ordinary symbols, it may be more futureproof solution, if we decide to add different diagnostics in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, there are more occasions where we copy the source code into json messages, see lsp_context::get_macro_documentation, and maybe some other places in lsp_context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, the only instance when we may end up including an invalid utf-8 sequences into diagnostics is when the code is generated (e.g. by substitutions).
In all other cases the text should be already sanitized or even parsed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes you are right, I forgot that we sanitize the source prior to parsing it.

How about values of variables that we send via DAP?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look into that one... 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, so dap variables are affected as well. But we could address that together with other dap issues that need to be solved.

@slavek-kucera slavek-kucera merged commit f272f7d into eclipse-che4z:development Aug 25, 2021
@slavek-kucera slavek-kucera deleted the sanitize_reparse_messages branch August 25, 2021 11:35
@github-actions
Copy link

github-actions bot commented Nov 3, 2021

🎉 This PR is included in version 0.15.0-beta.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Nov 8, 2021

🎉 This PR is included in version 0.15.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants