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

Changes towards a language server friendly compiler #673

Merged
merged 27 commits into from
Aug 19, 2024
Merged

Conversation

edusporto
Copy link
Contributor

This PR:

  • Changes all parsing to return TSPL::ParseError, new in the latest TSPL version, as to report error range information,
  • Adds optional FileSpan to each Diagnostic,
  • Adds the Function diagnostic origin,
  • Plenty of smaller changes to make the previous points work.

I'm still not sure about the TextSpan construct - the new TSPL version only reports byte range information, while the LSP requires line and column range information, so that's how I implemented that struct. I think it could be better to change TextSpan to only report byte range information, and provide a function to convert between the two given the source code. I could also change TSPL again to include both types of range information, but I think it's not something the team would want since many changes to it would stop it from being the simplest parser library.

I'll wait until after the review to add a changelog because of the aforementioned issues.

Related to #650.

src/diagnostics.rs Show resolved Hide resolved
src/imp/to_fun.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
cspell.json Outdated Show resolved Hide resolved
src/fun/mod.rs Outdated Show resolved Hide resolved
@edusporto
Copy link
Contributor Author

The latest commit includes the last suggested change - all diagnostic range and file information has been unified into the Source struct. This allowed me to change the way errors are printed to almost always include their source files, leading to MANY test result changes. If you'd like me to separate this into multiple pull requests so it's easier to review, let me know.

src/fun/mod.rs Outdated Show resolved Hide resolved
tests/snapshots/import_system__import_main.bend.snap Outdated Show resolved Hide resolved
tests/snapshots/readback_hvm__bad_net.bend.snap Outdated Show resolved Hide resolved
@edusporto
Copy link
Contributor Author

The new diagnostics printing should be much better.

Copy link
Member

@developedby developedby left a comment

Choose a reason for hiding this comment

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

Update the changelog

src/fun/parser.rs Show resolved Hide resolved
for diag in diags {
// We need to allow this Clippy warning due to `Name` in `DiagnosticOrigin::Function`.
// We know how it works, so it shouldn't be a problem.
#[allow(clippy::mutable_key_type)]
Copy link
Member

Choose a reason for hiding this comment

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

We can allow it at the crate level, since we're just sprinkling this macro in different places, always for the same reason.

@edusporto edusporto added this pull request to the merge queue Aug 19, 2024
Merged via the queue into main with commit bd592ca Aug 19, 2024
5 checks passed
@edusporto edusporto deleted the lsp-experiments branch August 19, 2024 13:08
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.

2 participants