Flatten nested highlight ranges during DFS traversal #3826

ltentrup · 2020-04-02T12:43:14Z

Implements the flattening of nested highlights from #3447.

There is a caveat: I needed to add Clone to HighlightedRange to split highlight ranges ~~and the nesting does not appear in the syntax highlighting test (it does appear in the accidental-quadratic test but there it is not checked against a ground-truth)~~.

I have added a test case for the example mentioned in #3447.

crates/ra_ide/src/syntax_highlighting.rs

kjeremy · 2020-04-02T12:59:55Z

crates/ra_ide/src/syntax_highlighting.rs


    let mut current_macro_call: Option<ast::MacroCall> = None;

    // Walk all nodes, keeping track of whether we are inside a macro or not.
    // If in macro, expand it first and highlight the expanded code.
    for event in root.preorder_with_tokens() {
+        assert!(!res.is_empty());


Should we have this assert! here?

The invariant is that the stack never becomes empty. This is asserted before and after the modification. I am fine with removing this assertion, I just usually err on the side of too many assertions 😉

kjeremy · 2020-04-02T13:03:54Z

Makes sense to me. I question the usefulness of assert!s but will defer that to @matklad.

One thing... how will this interact with the range LSP request? In theory I could see that splicing nested tokens.

ltentrup · 2020-04-02T13:54:35Z

This PR changes only the representation of the nesting and keeps the original logic untouched. Thus, the handling of range requests should be the same as before.

Veetaha · 2020-04-02T18:22:34Z

crates/ra_ide/src/syntax_highlighting.rs

+        assert!(!res.is_empty());
+        let current = res.last_mut().unwrap();


The server will panic on either line of code, I'd propose to replace assert!() with res.last_mut().expect("reason of the assert");

You're right, I'll update the PR with this change

ltentrup · 2020-04-03T07:52:34Z

I have updated the PR with the suggested changes and a new test case

matklad · 2020-04-03T14:23:18Z

crates/ra_ide/src/syntax_highlighting.rs

@@ -119,19 +161,20 @@ pub(crate) fn highlight(



When we add highlights below this line, I think we have an invariant that the highlight must not extend beyond the current node (otherwise, flattening might be broken). Let's add an assert for this.

matklad · 2020-04-03T14:23:50Z

crates/ra_ide/src/syntax_highlighting.rs

        }
    }

-    res
+    assert_eq!(res.len(), 1, "after DFS traversal, the stack should only contain a single element");


Let's also assert that ranges are sorted and disjoint

matklad · 2020-04-03T14:25:10Z

crates/ra_ide/src/syntax_highlighting.rs

+                 *
+                 * The following code implements the flattening, for our example this results to:
+                 * `[Attribute [0, 16), String [16, 21), Attribute [21, 23)]`
+                 */


After this, we should be able to remove the relevant logic from highlight_as_html function.

matklad · 2020-04-03T14:26:59Z

crates/ra_ide/src/syntax_highlighting/tests.rs

@@ -131,3 +131,19 @@ fn test_ranges() {

    assert_eq!(&highlights[0].highlight.to_string(), "field.declaration");
 }
+
+#[test]
+fn test_flattening() {


A good test here would be something that touches highlight_injection logic:

fn fixture(ra_fixture: &str) {} fn main() { fixture(r#" trait Foo { fn foo() { println!("2 + 2 = {}", 4); } } "#) }

This functionality is currently untested simply because naive flattening in highlight_as_html can't deal with it.

I wasn't aware of the highlight_injection logic, this is only a special case for rust-analyzer development, right? However, this is a great test for the flattening logic, otherwise, Rust does not seem to contain many nestings other than the attributes from the issue.
The current highlight_as_html logic iterates over both, the parsed tokens and the highlight ranges. With the change of the function type to be natively flat, it would be better to only iterate over the highlight ranges and consume the text file range by range. Is there an easy way to do that?

yup, this is only for rust-anayzer. And yes, you'd need to iteratete the text:

let text = parse.tree().syntax().to_string(); let mut curr = &text[..]; for range in ranges { curr = &curr[range.len..] }

matklad · 2020-04-03T14:36:41Z

I wonder if it makes sense to change the return type of the function to be natively flat. Like this:

#[derive(Debug)]
pub struct HighlightedRange {
    pub len: TextUnit,
    pub highlight: Option<Highlight>,
    pub binding_hash: Option<u64>,
}

I would probably go for it

…ogic

matklad · 2020-04-08T11:59:52Z

bors r+

Thanks!

bors · 2020-04-08T12:08:13Z

Build succeeded

3998: Make add_function generate functions in other modules via qualified path r=matklad a=TimoFreiberg Additional feature for #3639 - [x] Add tests for paths with more segments - [x] Make generating the function in another file work - [x] Add `pub` or `pub(crate)` to the generated function if it's generated in a different module - [x] Make the assist jump to the edited file - [x] Enable file support in the `check_assist` helper 4006: Syntax highlighting for format strings r=matklad a=ltentrup I have an implementation for syntax highlighting for format string modifiers `{}`. The first commit refactors the changes in #3826 into a separate struct. The second commit implements the highlighting: first we check in a macro call whether the macro is a format macro from `std`. In this case, we remember the format string node. If we encounter this node during syntax highlighting, we check for the format modifiers `{}` using regular expressions. There are a few places which I am not quite sure: - Is the way I extract the macro names correct? - Is the `HighlightTag::Attribute` suitable for highlighting the `{}`? Let me know what you think, any feedback is welcome! Co-authored-by: Timo Freiberg <[email protected]> Co-authored-by: Leander Tentrup <[email protected]> Co-authored-by: Leander Tentrup <[email protected]>

kjeremy reviewed Apr 2, 2020

View reviewed changes

crates/ra_ide/src/syntax_highlighting.rs Show resolved Hide resolved

kjeremy reviewed Apr 2, 2020

View reviewed changes

Veetaha reviewed Apr 2, 2020

View reviewed changes

Flatten nested highlight ranges during DFS traversal

bb45aca

ltentrup force-pushed the flatten-highlights branch from 1d666b2 to bb45aca Compare April 3, 2020 07:46

matklad reviewed Apr 3, 2020

View reviewed changes

Simplify HTML highlighter and add test case for highlight_injection l…

bf96d46

…ogic

bors bot merged commit 8ea7c9c into rust-lang:master Apr 8, 2020

kjeremy mentioned this pull request Apr 8, 2020

Flatten highlights #3447

Closed

ltentrup mentioned this pull request Apr 17, 2020

Syntax highlighting for format strings #4006

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flatten nested highlight ranges during DFS traversal #3826

Flatten nested highlight ranges during DFS traversal #3826

ltentrup commented Apr 2, 2020 •

edited

Loading

kjeremy Apr 2, 2020

ltentrup Apr 2, 2020

kjeremy commented Apr 2, 2020

ltentrup commented Apr 2, 2020

Veetaha Apr 2, 2020 •

edited

Loading

ltentrup Apr 3, 2020

ltentrup commented Apr 3, 2020

matklad Apr 3, 2020

matklad Apr 3, 2020

matklad Apr 3, 2020

matklad Apr 3, 2020 •

edited

Loading

ltentrup Apr 6, 2020

matklad Apr 6, 2020

matklad commented Apr 3, 2020

matklad commented Apr 8, 2020

bors bot commented Apr 8, 2020

		assert!(!res.is_empty());
		let current = res.last_mut().unwrap();

Flatten nested highlight ranges during DFS traversal #3826

Flatten nested highlight ranges during DFS traversal #3826

Conversation

ltentrup commented Apr 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjeremy commented Apr 2, 2020

ltentrup commented Apr 2, 2020

Veetaha Apr 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltentrup commented Apr 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad Apr 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Apr 3, 2020

matklad commented Apr 8, 2020

bors bot commented Apr 8, 2020

Build succeeded

ltentrup commented Apr 2, 2020 •

edited

Loading

Veetaha Apr 2, 2020 •

edited

Loading

matklad Apr 3, 2020 •

edited

Loading