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.
Details
- Reviewers
jpienaar - Commits
- rGed2fb1736ac1: [mlir:LSP] Add support for MLIR code completions
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/Parser/Parser.h | ||
---|---|---|
314 | This feels a bit weird. Is ParseResult correct type here? |
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. |
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? |
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 feels a bit weird. Is ParseResult correct type here?