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

Switch from codespan-reporting to a custom crate based on codespan #35

Merged
merged 51 commits into from
Oct 21, 2020

Conversation

Stupremee
Copy link
Member

This PR replaces codespan-reporting with annotate-snippets.
To actually use annotate-snippets in a nice manner and to make the switch easier,
a new module, rslint_errors, is introduced to provide an API similair to codespan-reporting
but build on top of annotate-snippets.

Resolves #29

@RDambrosio016
Copy link
Collaborator

A diagnostic struct with a lifetime is going to cause a myriad of issues, i pretty much rewrote half of the old linter because of it, so i would rather not do that

@RDambrosio016
Copy link
Collaborator

oops

@RDambrosio016 RDambrosio016 added A-errors C-enhancement Category: enhancement labels Oct 7, 2020
@Stupremee
Copy link
Member Author

What is the sense of the Applicability enum in your code in #29?

@RDambrosio016
Copy link
Collaborator

@Stupremee
Copy link
Member Author

Stupremee commented Oct 8, 2020

Do we really need the Applicability enum? I would not add it to RSLint because it doesn't seem to affect the error reporting

@RDambrosio016
Copy link
Collaborator

It does not affect error reporting but it does affect the LSP, since it needs to know what suggestions it can offer which dont have placeholders

@Stupremee
Copy link
Member Author

Stupremee commented Oct 9, 2020

Should we implement a similar suggestion API like rustc_errors? So you can "fix" the error by substituting some strings that are provided by the suggestion? I think this can be very nice in a linter. The parser can then try to fix many issues, that are obviously typos, by providing a suggestion, which will get automatically applied by auto-fix.

@RDambrosio016
Copy link
Collaborator

Yes the LSP can provide autofix options based on the suggestions from the diagnostic, although im not sure how we are going to do this. Id like to have a fixer for rules, but then the parser cant use it. I think we will keep the fixer for rules but have other suggestions to.

@RDambrosio016
Copy link
Collaborator

Eventually we will have to separate the AST and syntax node stuff into its own module, that way rslint_errors can provide a span for ast nodes and syntax nodes. That should allow us to make the fixer a standalone crate too, which means we can offer the fixer interface through rslint_errors for suggestions

@Stupremee
Copy link
Member Author

Stupremee commented Oct 10, 2020

So, I added all the basic structs and representations that now just need methods and logic to report.
I'm currently not sure if we should stay with SmolStr everywhere, or if we should use String.
I have no idea how much of a speed boost / memory saving SmolStr would give us.

@RDambrosio016
Copy link
Collaborator

SmolStr wouldnt do much, most messages are > 22 chars

@Stupremee
Copy link
Member Author

I think rslint_errors is ready to replace codespan now, except for suggestions, I have to fix some lifetime issues first, and I have to provide an immutable Diagnostic (or public methods to read the data) so that the LSP and other stuff can actually apply the suggestions. But other than that it should be ready.

@RDambrosio016
Copy link
Collaborator

I dont really see why the diagnostic's data should not be public

@Stupremee
Copy link
Member Author

Yea that works too

@Stupremee
Copy link
Member Author

Stupremee commented Oct 14, 2020

annotate-snippets uses lifetimes everywhere. That's sooo annoying. It blocks everything I want to do 😢

@RDambrosio016
Copy link
Collaborator

In what places does it cause issues?

@Stupremee
Copy link
Member Author

I have a method convert which will convert a Diagnostic to a Snippet, but all the strings used in Snippet have to be a &str and thus a lifetime is required, so i can't create new strings in the convert method and use them in a Snippet. But I think a found a fix.

@RDambrosio016 RDambrosio016 marked this pull request as ready for review October 19, 2020 03:32
@RDambrosio016
Copy link
Collaborator

The changes seem to work well, some of the rendering is wonky but we can fix that later, i think we can merge it now

@RDambrosio016
Copy link
Collaborator

I take that back, it seems we are suffering from the same issue as denoland/deno_lint#391 which is a little odd

@Stupremee
Copy link
Member Author

Stupremee commented Oct 19, 2020

The issues are super weird

Edit: I know the cause for both of them, I will fix them in our local copy.

@RDambrosio016
Copy link
Collaborator

Are you sure? it still seems to be a bit broke 🤔

error[SyntaxError]: expected `'('` but instead found `length`
  --> tests\main.js:20:7
   |
...
19 |
20 |   for length;
   |       ^^^^^^ unexpected
   |
error[SyntaxError]: Expected an expression, but found none
  --> tests\main.js:26:5
   |
...
25 |   // when jumpSize = √array.length.
26 |   const jumpSize = Math.floor(Math.sqrt(arraySize));
   |     ^^^^^ Expected an expression here
   |

@RDambrosio016
Copy link
Collaborator

Switching from annotate-snippets to codespan has solved all of the issues, essentially the rendering is the same as using codespan on its own but we have a cool new unified builder as well as suggestions, and with all of the nice codespan rendering 🎉

@RDambrosio016 RDambrosio016 changed the title Switch from codespan-reporting to a custom crate based on annotate-snippets Switch from codespan-reporting to a custom crate based on codespan Oct 21, 2020
@RDambrosio016 RDambrosio016 merged commit 253508d into rslint:master Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-errors C-enhancement Category: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider switching from codespan-reporting to annotate-snippets
2 participants