ra_syntax: reshape SyntaxError for the sake of removing redundancy #3026

Veetaha · 2020-02-06T00:40:08Z

Followup of #2911, also puts some crosses to the todo list of #223.

AHTUNG! A big part of the diff of this PR are test data files changes.

Simplified SyntaxError that was SyntaxError { kind: { /* big enum */ }, location: Location } to SyntaxError(String, TextRange). I am not sure whether the tuple struct here is best fit, I am inclined to add names to the fields, because I already provide getters SyntaxError::message(), SyntaxError::range().
I also removed Location altogether ...

This is currently WIP, because the following is not done:

Add tests to test_data dir for unescape errors // I don't know where to put these errors in particular, because they are out of the scope of the lexer and parser. However, I have an idea in mind that we move all validators we have right now to parsing stage, but this is up to discussion... [UPD] I came to a conclusion that tree validation logic, which unescape errors are a part of, should be rethought of, we currently have no tests and no place to put tests for tree validations. So I'd like to extract potential redesign (maybe move of tree validation to ra_parser) and adding tests for this into a separate task.

Veetaha · 2020-02-06T00:44:08Z

crates/ra_syntax/src/parsing/reparsing.rs

 ) -> Vec<SyntaxError> {
    let mut res = Vec::new();
-    for e in old_errors {
-        if e.offset() <= old_range.start() {


AHTUNG! I suppose in this if statement we had a mistake, we need to check that e.range().end() <= old_range.start() and I corrected it in this PR (tests pass anyway...)

Veetaha · 2020-02-06T00:47:45Z

crates/ra_syntax/src/parsing/reparsing.rs

+    res.extend(new_errors.into_iter().map(|new_err| {
+        // fighting borrow checker with a variable ;)
+        let offseted_range = *new_err.range() + range_before_reparse.start();
+        new_err.with_range(offseted_range)


I am surprised how this reports a move after borrow error

new_err.with_range(*new_err.range() + range_before_reparse.start()) // where fn with_range(mut self, range: TextRange) -> Self; fn range(&self) -> &TextRange;

Veetaha · 2020-02-06T00:49:35Z

crates/ra_syntax/src/syntax_error.rs

+pub struct SyntaxError(String, TextRange);
+
+// FIXME: there was an unused SyntaxErrorKind previously (before this enum was removed)
+// It was introduced in this PR: https://github.com/rust-analyzer/rust-analyzer/pull/846/files#diff-827da9b03b8f9faa1bade5cdd44d5dafR95


FYI: there was an enum variant of SyntaxError that was not used anywhere, it was introduced in this PR, but then match arms attributes validation logic was removed without this variant being deleted...

Veetaha · 2020-02-06T00:56:45Z

crates/ra_syntax/src/parsing/reparsing.rs

+
+    for old_err in old_errors {
+        let old_err_range = *old_err.range();
+        // FIXME: make sure that .start() was here previously by a mistake


This fixme will be removed before merging this PR

It should be TODO then

Yes, I just temporarily marked it FIXME to let it pass no todo tests for a while.

matklad · 2020-02-06T10:04:50Z

A big part of the diff of this PR are test data files changes.

A nice trick here to commit code changes and test data changes in sepparate commits, so that the diff is easier to review.

matklad · 2020-02-06T10:12:02Z

crates/ra_syntax/src/parsing/reparsing.rs

+
+    for old_err in old_errors {
+        let old_err_range = *old_err.range();
+        // FIXME: make sure that .start() was here previously by a mistake


It should be TODO then

crates/ra_syntax/src/syntax_error.rs

matklad · 2020-02-06T10:37:56Z

crates/ra_syntax/src/syntax_error.rs

@@ -1,209 +1,47 @@
-//! FIXME: write short doc here
+//! Module that defines `SyntaxError`.


Would be better to explain why, rather than what:

This module defines SyntaxError, the type we use to represent errors from

tokenization

parsing

tree validation

The errors are unstructured: just a message and a location.

I moved this description to doc comment on top of SyntaxError itself, otherwise it would be repetition

crates/ra_syntax/src/syntax_error.rs

matklad · 2020-02-06T10:40:29Z

crates/ra_syntax/src/syntax_error.rs


-    pub fn location(&self) -> Location {
-        self.location.clone()
+    pub fn message(&self) -> &str {


We should remove this function and just impl fmt::Display

Hmm, yes, seems reasonable.

I would expect an fmt::Display impl to give both the error message and location, while this method only gives the message, not the location.

You never see error location displayed as a hint on top of error squiggles, but you do want to see the text range if you print it to stdout. In case of std::fmt::Display this is for the first case, whilst std::fmt::Debug is for the second case. But anyway, I think this is not transparent, should we remove impl Display at all here then?

I did remove message() function, though this is a bit controversial, but I'd like to trust matklad here. Maybe I am wrong saying not transparent in my previous comment right here...

matklad · 2020-02-06T10:40:46Z

crates/ra_syntax/src/syntax_error.rs

-            Location::Offset(offset) => offset,
-            Location::Range(range) => range.start(),
-        }
+    pub fn range(&self) -> &TextRange {


No need to return a reference, TextRange is small and Copy

Yeah, I am just used to getters returning by reference, wanted to hear your thoughts.

matklad · 2020-02-06T10:45:13Z

crates/ra_syntax/src/syntax_node.rs

-    Parse, SmolStr, SyntaxKind, TextUnit,
-};
-
-pub(crate) use rowan::{GreenNode, GreenToken};


The "re-export" was intentionally after "imports", but I dont' have strong feelings about htis.

Hmm, if I am not mistaken (need to recheck), this is what rustfmt suggested me to do and I couldn't resist

matklad · 2020-02-06T11:04:43Z

No strong opinion here, but I prefer to document modules, rather than structs, because they are more useful for high-level picture.

…

On Thu, 6 Feb 2020 at 12:01, Veetaha ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In crates/ra_syntax/src/syntax_error.rs <#3026 (comment)> : > @@ -1,209 +1,47 @@ -//! FIXME: write short doc here +//! Module that defines `SyntaxError`. I moved this description to doc comment on top of SynraxError itself, otherwise it would be repetition — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#3026>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3M5RXDLNDUEYVN7ZGE3RBPU2FANCNFSM4KQUPYHQ> .

Veetaha · 2020-02-06T14:26:43Z

@matklad

No strong opinion here, but I prefer to document modules, rather than
structs, because they are more useful for high-level picture.

https://github.com/rust-analyzer/rust-analyzer/blob/355c98fd0861acf0f0fddad08cbc923fee0698fb/crates/ra_ide_db/src/feature_flags.rs#L1-L16

But I see here you decided to document a struct instead of a module ...

matklad · 2020-02-07T09:07:43Z

I cannot be right because of that specific reason: my point was rather that we should now allow UI concerns (how exactly is the red squiggle shown) bleed into domain model. If we want to customize the squiggle, the place to do that is conv.rs all the way up in ra+lsp-server. But yeah, it is convenient that vscode just does the right thing out of the box here,

…

On Friday, 7 February 2020, Veetaha ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In crates/ra_syntax/src/syntax_error.rs <#3026 (comment)> : > } - - pub fn kind(&self) -> SyntaxErrorKind { - self.kind.clone() + pub fn new_at_offset(message: impl Into<String>, offset: TextUnit) -> Self { + Self(message.into(), TextRange::offset_len(offset, 1.into())) Apparently you are right, this is what I found in vscode docs <https://code.visualstudio.com/api/language-extensions/language-server-extension-guide#diagnostics-tips-and-tricks> If the start and end positions are the same, VS Code will underline with a squiggle the word at that position. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3026>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3MZSK72ISQXLC445AFDRBUPUJANCNFSM4KQUPYHQ> .

Veetaha · 2020-02-11T17:25:14Z

@matklad, I know this is not the most important thing, but might I ask you, please to elaborate on this comment?

matklad · 2020-02-11T17:27:16Z

Still no strong opinion, though I find there's a meaningful difference between "See X" (which is a redirect) and "This module defines X" (which reads like a placeholder comment) :)

Veetaha · 2020-02-11T17:34:41Z

But I do have a strong opinion. When you use the struct itself, you get documentation popups with more detailed docs, but you don't get them when you trigger autocompletion for the module itself (mentioning that you often just use module and never trigger completion for it thereafter), (maybe this is a bug, that docs don't appear on modules?)

Veetaha · 2020-02-11T17:35:20Z

See X sounds better, you are right!

matklad · 2020-02-11T17:35:35Z

Yeah, convinced, you are right! We should move all single-struct modules to See X style!

Co-Authored-By: Aleksey Kladov <aleksey.kladov@gmail.com>

…value

… X" as per matklad

…axError as per matklad

…en constructed from TextUnit

…orts errors

Veetaha · 2020-02-18T00:15:00Z

@matklad, in response to your comment here about the bug I supposedly found.

I did check that there was a bug in reparsing::merge_errors() code. I added 4 tests that don't pass with previous code, but pass with my fixes in this commit: 053ccf4

Also, note that, though unintended to be in this PR, but I was forced to amend reparse_token() code to return errors so that tests pass (because lexer didn't report errors previously and reparsing code ignored them, though until now).

As a side note, we have a bit of code duplication in incremental_reparse() function, but I wouldn't like to tackle this in this PR and anytime soon at all...

Veetaha · 2020-02-18T00:24:25Z

Also, @matklad, please note on an [UPD] I wrote in PR description, I hope it makes sense. I'd like to experiment with moving tree validation errors to ra_parser and discuss the outcome of my experiments, after that we may redesign this and add tests for the new design, or leave it as it is and add tests for existing design later.

I hope it makes sense!

matklad · 2020-02-18T12:46:51Z

I'd like to experiment with moving tree validation errors to ra_parser

ra_parser by design does not have access to ast:: types, but we probably want to implement validations in terms of typed AST

matklad · 2020-02-18T12:57:10Z

bors r+

3026: ra_syntax: reshape SyntaxError for the sake of removing redundancy r=matklad a=Veetaha Followup of #2911, also puts some crosses to the todo list of #223. **AHTUNG!** A big part of the diff of this PR are test data files changes. Simplified `SyntaxError` that was `SyntaxError { kind: { /* big enum */ }, location: Location }` to `SyntaxError(String, TextRange)`. I am not sure whether the tuple struct here is best fit, I am inclined to add names to the fields, because I already provide getters `SyntaxError::message()`, `SyntaxError::range()`. I also removed `Location` altogether ... This is currently WIP, because the following is not done: - [ ] ~~Add tests to `test_data` dir for unescape errors *// I don't know where to put these errors in particular, because they are out of the scope of the lexer and parser. However, I have an idea in mind that we move all validators we have right now to parsing stage, but this is up to discussion...*~~ **[UPD]** I came to a conclusion that tree validation logic, which unescape errors are a part of, should be rethought of, we currently have no tests and no place to put tests for tree validations. So I'd like to extract potential redesign (maybe move of tree validation to ra_parser) and adding tests for this into a separate task. Co-authored-by: Veetaha <gerzoh1@gmail.com> Co-authored-by: Veetaha <veetaha2@gmail.com>

Veetaha · 2020-02-18T12:59:24Z

I would not be 100% sure that we can't go without typed asts in a nice way, I'll look over it and make a conclusion...

bors · 2020-02-18T13:03:25Z

Build succeeded

Rust (macos-latest)
Rust (ubuntu-latest)
Rust (windows-latest)
TypeScript

Veetaha · 2020-02-18T13:03:38Z

Oh, I just noticed I forgot to remove one FIXME comment...

matklad · 2020-02-18T13:05:44Z

And that's why one should use TODO for comments that *should* be removed before merging :D

…

On Tue, 18 Feb 2020 at 14:03, Veetaha ***@***.***> wrote: Oh, I just noticed I forgot to remove one FIXME comment... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3026>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3M77IGYZZSSTM24LYCDRDPMCVANCNFSM4KQUPYHQ> .

Veetaha requested review from aochagavia, kiljacken and matklad and removed request for kiljacken February 6, 2020 00:41

Veetaha commented Feb 6, 2020

View reviewed changes

Veetaha force-pushed the feature/reshape-syntax-error branch 2 times, most recently from 076d830 to 34158f1 Compare February 6, 2020 00:55

Veetaha commented Feb 6, 2020

View reviewed changes

matklad reviewed Feb 6, 2020

View reviewed changes

Veetaha mentioned this pull request Feb 12, 2020

Init implementation of structural search replace #3099

Merged

Veetaha and others added 7 commits February 17, 2020 22:24

ra_syntax: reshape SyntaxError for the sake of removing redundancy

9fdf984

Change semmantical analyzis to tree validation as per @matklad

c582766

Co-Authored-By: Aleksey Kladov <aleksey.kladov@gmail.com>

ra_syntax: Apply Aleksei Kladov suggestion about TextRange of TextUnit

6ae4850

Co-Authored-By: Aleksey Kladov <aleksey.kladov@gmail.com>

ra_syntax: moved reexport declaration after imports as per matklad

acdab6f

ra_syntax: SyntaxError::range() now returns by value

e00922d

ra_syntax: followup fix for making SyntaxError::range() to return by …

b510e77

…value

ra_syntax: change module-level documentation for syntax_error to "see…

e05eb63

… X" as per matklad

Veetaha force-pushed the feature/reshape-syntax-error branch from 43df237 to e05eb63 Compare February 17, 2020 20:51

ra_syntax: remove message() method and use only Display trait in Synt…

cd8e56c

…axError as per matklad

Veetaha added 2 commits February 17, 2020 23:14

ra_syntax: migrate test data to 0-length text-range of SyntaxError wh…

fc5e7b8

…en constructed from TextUnit

ra_syntax: fix reparsing merging errors, also now reparse_token() rep…

053ccf4

…orts errors

Veetaha marked this pull request as ready for review February 18, 2020 00:11

Veetaha requested review from bjorn3 and matklad February 18, 2020 00:28

bors bot merged commit 053ccf4 into rust-lang:master Feb 18, 2020

Veetaha deleted the feature/reshape-syntax-error branch February 24, 2020 20:11

		@@ -1,209 +1,47 @@
		//! FIXME: write short doc here
		//! Module that defines `SyntaxError`.

ra_syntax: reshape SyntaxError for the sake of removing redundancy #3026

ra_syntax: reshape SyntaxError for the sake of removing redundancy #3026

Uh oh!

Conversation

Veetaha commented Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matklad commented Feb 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Veetaha Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjorn3 Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Veetaha Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matklad commented Feb 6, 2020 via email

Uh oh!

Veetaha commented Feb 6, 2020

Uh oh!

matklad commented Feb 7, 2020 via email

Uh oh!

Veetaha commented Feb 11, 2020

Uh oh!

matklad commented Feb 11, 2020

Uh oh!

Veetaha commented Feb 11, 2020

Uh oh!

Veetaha commented Feb 11, 2020

Uh oh!

matklad commented Feb 11, 2020

Uh oh!

Veetaha commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Veetaha commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matklad commented Feb 18, 2020

Uh oh!

matklad commented Feb 18, 2020

Uh oh!

Veetaha commented Feb 18, 2020

Veetaha commented Feb 6, 2020 •

edited

Loading

Veetaha Feb 6, 2020 •

edited

Loading

bjorn3 Feb 6, 2020 •

edited

Loading

Veetaha Feb 6, 2020 •

edited

Loading

Veetaha commented Feb 18, 2020 •

edited

Loading

Veetaha commented Feb 18, 2020 •

edited

Loading