Page MenuHomePhabricator

[clangd] Introduce intermediate representation of formatted text
ClosedPublic

Authored by ilya-biryukov on Feb 22 2019, 7:21 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.Feb 22 2019, 7:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2019, 7:21 AM

This is a follow-up for a discussion from D55250.
Still missing test, wanted to get some input on the API first.

sammccall edited reviewers, added: kadircet; removed: ioeric.May 3 2019, 12:15 AM

This looks pretty nice to me! Sorry for the wait.
Adding @kadircet as hover-guru-in-waiting.

clang-tools-extra/clangd/ClangdLSPServer.cpp
814 ↗(On Diff #187936)

(I'd like to see that in this patch if possible, it shouldn't be much work)

clang-tools-extra/clangd/ClangdServer.h
186 ↗(On Diff #187936)

Not sure about switching from Hover to FormattedString as return type here: LSP hover struct contains a range field (what are the bounds of the hovered thing?) that we may want to populate that doesn't fit in FormattedString.
I'd suggest the function in XRefs.h should return FormattedString (and later maybe pair<FormattedString, Range>), and the ClangdServer version should continue to provide the rendered Hover.

(We'll eventually want ClangdServer to return some HoverInfo struct with structured information, as we want embedding clients to be able to render it other ways, and maybe use it to provide extension methods like YCM's GetType. But no need to touch that in this patch, we'll end up producing HoverInfo -> FormattedString -> LSP Hover, I guess)

clang-tools-extra/clangd/FormattedString.cpp
41 ↗(On Diff #187936)

you may want to take the language name, and default it to either cpp or nothing.
Including it in the triple-backtick block can trigger syntax highlighting, and editors are probably somewhat likely to actually implement this.

63 ↗(On Diff #187936)

why do we add surrounding spaces if we don't do the same for text chunks?

clang-tools-extra/clangd/FormattedString.h
1 ↗(On Diff #187936)

This will need tests as you note, which shouldn't be hard.
It's not terribly complicated, but it's the sort of thing that may accumulate special cases.

27 ↗(On Diff #187936)

I guess this will get an optional parameter to control the style (bold, italic etc)?

31 ↗(On Diff #187936)

Having various methods that take strings may struggle if we want a lot of composability (e.g. a bullet list whose bullet whose third word is a hyperlink/bold).

(I'm not sure whether this is going to be a real problem, just thinking out loud here)

One alternative extensibility would be to make "containers" methods taking a lambda that renders the body.
e.g.

FormattedString S;
S << "std::";
S.bold([&} S << "swap" };
S.list(FormattedString::Numbered, [&] {
  S.item([&] { S << "sometimes you get the wrong overload"; });
  S.item([&] {
    S << "watch out for ";
    S.link("https://en.cppreference.com/w/cpp/language/adl", [&] { S << "ADL"; });
  });
});
S.codeBlock([&] S << "Here is an example"; });

Not sure if this is really better, I just have it on my mind because I ended up using it for the JSON.h streaming output. We can probably bolt it on later if necessary.

39 ↗(On Diff #187936)

I think we want renderForDebugging() or so which would print the chunks explicitly, a la CodeCompletionString.

This is useful for actual debugging, and for testing methods that return FormattedString (as opposed to the FormattedString->markdown translation)

clang-tools-extra/clangd/XRefs.cpp
528 ↗(On Diff #187936)

should this be an inline code block?

clang-tools-extra/unittests/clangd/XRefsTests.cpp
1157 ↗(On Diff #187936)

When this lands for real, I think we should assert on the renderForDebugging() output or similar.

kadircet added inline comments.May 3 2019, 3:38 AM
clang-tools-extra/clangd/ClangdServer.h
186 ↗(On Diff #187936)

I agree with Sam on the layering. I was also working in a patch that was changing ClangdServers output from Hover to HoverInfo(a structured version of Hover).

ilya-biryukov marked 7 inline comments as done.
  • Rebase
  • Parse client capabilities, send markdown on hover if client supports it
  • Use inline block for the scope
  • Add a language for code blocks
  • Add a FIXME for hover tests
  • Fix escaping, add tests
  • Update XRefs tests
ilya-biryukov marked an inline comment as done.May 3 2019, 6:23 AM
ilya-biryukov added inline comments.
clang-tools-extra/clangd/FormattedString.cpp
41 ↗(On Diff #187936)

Done. Defaulting to 'cpp'.
VSCode actually does syntax highlighting there and it looks nice.

63 ↗(On Diff #187936)

a leftover from my first prototype.
They don't seem to be necessary, removed them.

clang-tools-extra/clangd/FormattedString.h
31 ↗(On Diff #187936)

Agree that the current approach won't generalize to more complicated cases without some design work.
The lambda-heavy code is hard to read to my personal taste, but may actually be a good option in practice.
I'd also try playing around with some lambda-free alternatives to see if they lead to better syntax without compromising extensibility or making the implementation overly complex.

I also think we should bolt on this until we hit more use-cases with more extensive needs for formatting markdown.

39 ↗(On Diff #187936)

That would be a useful addition. I've added a FIXME for now and testing plaintext in XRefs.
I'd suggest adding debug representation in a follow-up change, but also happy to do it now if you feel that's necessary.

That would need updates to all hover tests and that's a big enough to go into a separate change, from my perspective.

ilya-biryukov marked an inline comment as done.May 3 2019, 6:27 AM
ilya-biryukov added inline comments.
clang-tools-extra/clangd/ClangdServer.h
186 ↗(On Diff #187936)

ClangdServer does not know whether to render the FormattedString into markdown or plaintext and I believe it should stay that way.
Happy to return HoverInfo, though. I'll introduce one with FormattedString and Range

ilya-biryukov marked an inline comment as done.
  • Return 'HoverInfo' instead of FormattedString
  • Reformat
clang-tools-extra/clangd/ClangdServer.h
186 ↗(On Diff #187936)

Returning HoverInfo now, with a FormattedString and a range.

  • Remove a FIXME that was fixed
Harbormaster completed remote builds in B31364: Diff 197991.
ilya-biryukov marked 2 inline comments as done.May 3 2019, 6:36 AM
ilya-biryukov added inline comments.
clang-tools-extra/clangd/FormattedString.h
27 ↗(On Diff #187936)

Either that or a separate function to add bold/italic, etc.
I think we should keep it simple in the first revision.

This really tries to provide a vocabulary type to carry a string with formatted blocks around, not support full markdown from the start.

ilya-biryukov marked an inline comment as done.
  • Remove dead test code. NFC

This is ready for another round of review. As mentioned in the comments, I think some of the comments are better addressed by a separate change, because they would touch lots of lines in the test.
Let me know if I missed anything else.

As discussed, it probably makes sense to split into two patches, adding this abstraction and using it.
(All together is also fine but should have the tests for the new behavior, which probably means renderForTests() too)

Generally looks good. I think we'll run into the limitations as soon as we want to add lists, but that's OK.

clang-tools-extra/clangd/FormattedString.cpp
71 ↗(On Diff #197994)
backticks = "```"
while (Input.contains(backticks))
  backticks.push_back("`")

I don't think we care about DOS here?

90 ↗(On Diff #197994)

comment for this merge?

107 ↗(On Diff #197994)

I'm not sure we're realistically going to be able to enforce this very well, we're going to use this for types. Maybe translate into ' ' instead? (or at least in production mode)

140 ↗(On Diff #197994)

(we could also use the version in CharInfo)

ilya-biryukov marked 7 inline comments as done.
  • Use isWhitespace from CharInfo.
  • Add a comment for merging text blocks together.
  • Remove assertion that inline code block does not contain newlines.
  • Simplify how the max number of backticks is computed.
clang-tools-extra/clangd/FormattedString.cpp
71 ↗(On Diff #197994)

I'd like to keep this linear anyway.
Simplified the algorithm a bit, let me know if it still looks too complicated.

107 ↗(On Diff #197994)

Actually, the CommonMark specification allows newline in the inline code blocks.
The renderers should replace newlines with spaces.
Removed the assertion.

140 ↗(On Diff #197994)

Thanks! I knew there should be a helper like this somewhere in LLVM...

  • Remove hover-related bits, they will go into a separate revision
sammccall accepted this revision.May 7 2019, 6:42 AM
sammccall added inline comments.
clang-tools-extra/clangd/FormattedString.h
34 ↗(On Diff #198300)

newlines are allowed now?

This revision is now accepted and ready to land.May 7 2019, 6:42 AM
ilya-biryukov marked an inline comment as done.
  • Update a comment
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2019, 7:15 AM