This is an archive of the discontinued LLVM Phabricator instance.

[mlir:LSP] Add support for MLIR code completions
ClosedPublic

Authored by rriddle on Jul 6 2022, 3:35 AM.

Details

Summary

This commit adds code completion results to the MLIR LSP using
a new code completion context in the MLIR parser. This commit
adds initial completion for dialect, operation, SSA value, and
block names.

Diff Detail

Event Timeline

rriddle created this revision.Jul 6 2022, 3:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2022, 3:35 AM
rriddle requested review of this revision.Jul 6 2022, 3:35 AM
jpienaar added inline comments.Jul 6 2022, 6:08 AM
mlir/lib/Parser/Parser.h
314

This feels a bit weird. Is ParseResult correct type here?

rriddle added inline comments.Jul 6 2022, 10:29 AM
mlir/lib/Parser/Parser.h
314

Either ParseResult or LogicalResult is what we want. We really just want a silent failure that stops the parsing process, given that reaching a code completion point is generally a sign that we've done all that we wanted to do during parsing.

rriddle updated this revision to Diff 442836.Jul 7 2022, 3:12 AM
rriddle edited the summary of this revision. (Show Details)
jpienaar accepted this revision.Jul 7 2022, 8:29 AM

Looks good in general thanks!

mlir/lib/Parser/Parser.h
314

What does failure signify here? That it did successfully code complete? Why not have it something like "bool hasCodeCompletedDialectName()" and then at the call site decide what to do with it? (this reminds me too much of Status situations where folks want to use RETURN_IF_ERROR macro for convenience and so leaf functions end up with Status return types while they aren't really exceptional cases, and so this may irritate me more than most readers :)). Returning failure on success feels weird, it makes sense at the caller, but for the function itself it feels odd as failure is successfully performed task, and success means nothing happened. [not blocker]

mlir/lib/Tools/mlir-lsp-server/MLIRServer.cpp
643

What does this do? Just give ordering preference when returning completions?

707

Why is this needed? Is completion just heavier and want to avoid it unless requested? Or is it because we abort on complete above that we couldn't do it always?

717

kwargs style comment for nullptr?

This revision is now accepted and ready to land.Jul 7 2022, 8:29 AM
rriddle marked 5 inline comments as done.Jul 7 2022, 11:10 AM
rriddle added inline comments.
mlir/lib/Parser/Parser.h
314

It's less about the success of completion, completion is essentially just enqueueing something in the LSP, and more about convenience and keeping the meaning of the result/failure specific to the completion methods. I agree that we are using "failure" here, but what we really want to do is interrupt the parser process (given that these methods always "succeed"). One alternative would be sprinkle return failure() in each place this is called, but that one doesn't seem as nice to me given that here we can document and define what the failure is supposed to mean (plus it's not as clean). Another would be to have a special/local result type used here that is self documenting. I have a slight preference to keep as-is, given that this aligns with how we ended up doing it in PDLL, but happy to change if there is a strong preference otherwise.

mlir/lib/Tools/mlir-lsp-server/MLIRServer.cpp
643

Yeah, sortText is used to determine the ordering of the results (if not present, the label field gets used for this).

707

The latter for a couple reasons. "Code completion" is a "special" mode and we want to be able to inject completions that can only come from being in the immediate context (e.g. keywords, dialect names, etc.). Aside from that, completion often runs on incomplete IR (given that it runs while you type), so we want to be able to build tokens in special ways depending on where the completion location is.

717

Thanks for the catch.

This revision was automatically updated to reflect the committed changes.
rriddle marked 4 inline comments as done.