Stream: t-compiler/wg-rls-2.0

Topic: folding in vscode


ztlpn (Oct 21 2019 at 14:30, on Zulip):

Hi @matklad are you available for a quick chat on https://github.com/rust-analyzer/rust-analyzer/issues/2033 ?

matklad (Oct 21 2019 at 14:31, on Zulip):

yeah!

ztlpn (Oct 21 2019 at 14:39, on Zulip):

Nice, so it turns out folding in vscode is pretty terrible with complaints going all the way to 2016 :) Basically it will slurp up the full last line of the folding range. First thing I tried was excluding it unconditionally - but the downside is that folds of modules and comments start looking really strange. Second idea - exclude the last line only if the folding range ends somewhere before the line end. This kind of works (see https://github.com/ztlpn/rust-analyzer/commit/173ac3d367e26c4ceb20f24b62e25f78a94e4517) modulo corner cases like trailing whitespace. Today I had another idea - add a field named something like next_non_whitespace_elt_start to struct Fold and calculate it along with the folding regions (where the full syntax tree is available). Seems a bit strange to do it only for that purpose but looks like it is the cleanest way. What do you think?

matklad (Oct 21 2019 at 14:44, on Zulip):

Interesting!

I think it's not the best choice to put "this this otherwise blank line?" logic to the code that computes folding regions themselves. THe code in ra_ide_api is expcted to be independent of the LSP layer. That is, the hope is that it will be usable even without LSP.

So, unless it is absolutely horrible, I would prefer to do this calculation in the conv layer. One thing which might help here is to actually pass in the text of the file to conv? That way, you can do something like text[fold..].chars().take_while(|it| it !='\n').filter(|it| !it.is_whitespace()).count() > 0

matklad (Oct 21 2019 at 14:45, on Zulip):

And I think that we want to check if fold is also the start of the line?

matklad (Oct 21 2019 at 14:46, on Zulip):

but in general, patch https://github.com/ztlpn/rust-analyzer/commit/173ac3d367e26c4ceb20f24b62e25f78a94e4517 looks good to me

matklad (Oct 21 2019 at 14:47, on Zulip):

Like, it's not the most pleasant code I've seen, but it is restricted strictly to this thin conversion layer, so supporting it shouldn't be too bad.

Jeremy Kolb (Oct 21 2019 at 14:49, on Zulip):

Huh... I didn't realize that folding range took line endings into account

ztlpn (Oct 21 2019 at 14:50, on Zulip):

One thing which might help here is to actually pass in the text of the file to conv?

Yeah, that would help. The question is - how to get it? A separate query?

matklad (Oct 21 2019 at 14:50, on Zulip):

Yeah, that's one bit that seemed a little eyebrow-rasing for me. I think if we switch to looking at the text directly, than we wouldn't need to look at line endings

matklad (Oct 21 2019 at 14:51, on Zulip):

@ztlpn Analysis::file_text

ztlpn (Oct 21 2019 at 14:51, on Zulip):

Ok, good

ztlpn (Oct 21 2019 at 14:52, on Zulip):

Another question - how do I test it?

ztlpn (Oct 21 2019 at 14:52, on Zulip):

heavy test seems a bit, well, heavy

matklad (Oct 21 2019 at 14:54, on Zulip):

@ztlpn in general, we don't test conv layer heavily: what matters is how the over side interprets converted things, so tests are not super-helpful in that case.

Here', I'd extract this into a helper function and test that

fn fixup_folding_ranges_for_editors_which_cant_fold_lines_partially(
    folds: &mut [Fold],
    file_text: &str
) {
    ....
}
ztlpn (Oct 21 2019 at 14:57, on Zulip):

Sure

ztlpn (Oct 21 2019 at 14:57, on Zulip):

Thanks

matklad (Oct 21 2019 at 15:03, on Zulip):

@ztlpn this might be confusing, so let me explicitelly mention just in case: the text you get from file_text is guaranteed to contain only \n line endings

ztlpn (Oct 21 2019 at 15:06, on Zulip):

Good to know, but I think in this case it doesn't matter? '\r' is whitespace too :)

matklad (Oct 21 2019 at 15:09, on Zulip):

Yeah, it shouldn't matter

Last update: Nov 12 2019 at 16:35UTC