[PATCH] D99540: [clangd] Preserve diags between tweak enumeration and execution

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.
Herald added a subscriber: cfe-commits.

Sorry about taking forever to get around to this, I kept looking at it and not understanding it.

IIUC the idea is:

- tweaks depend on various data from the codeAction request
- but we only have the codeAction request when enumerating, not when executing
- so we jam all the data needed to recalculate the tweak into the command args
- now tweaks depend on codeactioncontext.diagnostics, so jam that in command args too

**This definitely works and we can move forward with it** but it raises some ideas I'd like to talk through...

---

Extending this pattern this way feels wasteful: while file/range/tweakID is always needed to recompute the tweak, diagnostics almost never are. This inefficiency probably **isn't** really important (though amusingly if you have N diagnostics overlapping a point where M code actions are available, the `codeAction` response will contain N*M diagnostics...)
And the advantage is, the logic to extract info out of diagnostics gets exactly reused between enumerate & apply, which is the Tweak design.

But it's worth **considering** the alternative. Fundamentally this is that enumerate extracts the info from diagnostics and passes it directly to apply, which is the Code Action design. In this world the tweak only sees the CodeActionContext when enumerating and can output some JSON data, and when applying it sees the JSON data instead.

The reason this is worth entertaining is the Code Action design supports multiple instances of the same tweak, where the Tweak design does not. To support three different "override abstract method X" actions, each needs to pass its own value of X as data from enumerate -> apply so that they end up doing different things. And if we had this mechanism, I think would feel like the natural way to handle diagnostics.

I think this probably looks something like

  class Tweak {
    virtual bool prepare(const Selection&); // as now, if false then we can't use this tweak
    struct Case { std::string title; json::Object Data };
    vector<Case> cases(); // replaces title()
    Expected<Effect> apply(json::Object Data);
  }
  // enumeration = prepare() + cases()
  // application = prepare() + apply()

Maybe it makes sense to drop prepare() and have tweaks factor out any common logic between cases & apply in whatever way suits them.

---

Anyway this is obviously yet another big yak-shave and this patch isn't the place to discuss it, but I wanted to brain dump a bit. Let's talk next week. Like I said we can go ahead with the approach in this patch, but I want to understand the long-term.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99540/new/

https://reviews.llvm.org/D99540