[PATCH] D78338: [clangd] Enable diagnostic fixes within macro argument expansions.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:563
+    for (auto &FixIt : FixIts) {
+      // Allow fixits within a single macro-arg expansion to be applied.
+      if (FixIt.RemoveRange.getBegin().isMacroID() &&
----------------
hokein wrote:
> I feel a bit nervous about this (even it is for macro-arg expansion only), as macro is very tricky.
> 
> I think we may result in invalid code after applying the fixits in some cases:
> 
> 1) if the fix is to remove an unused variable (interestingly, clang doesn't provide fixit to remove an unused variable, but for unused lambda capture, it does)
> 
> ```
> #define LA(arg1, arg2) [arg1, arg2] { return arg2;}
> void test1(int x, int y) {
>   LA(x, y); // after fixit, it would become LA(, y)? or LA(y)
> }
> ```
> 
> 2) if the fix is to add some characters to the macro argument, e.g. adding a dereference `*`, the semantic of the code after macro expansion maybe changed.
> 
> ```
> void f1(int &a);
> void f2(int *a) {
>    f1(a); // clang will emits a diagnostic with a fixit adding preceding a `*` to a.
> }
> ```
> 
> maybe we should be more conservative? just whitelist some diagnostics? fixing typos seems pretty safe.
your `test1` example doesn't trigger this case because the fix has to delete a comma that's provided by the macro body - this patch doesn't change behavior.

To construct an example that follows this schema:
```
struct S { S(int *x); };
int *x;
S s1(*x); // fixit -> S s1(x);
#define CONCAT(a,b) a b
S s2(CONCAT(*, x)); // fixit -> S s2(CONCAT(, x));
```

The fixed code compiles fine and addresses the error in the expected way. It may not be *idiomatic*, but this is also a pathological case. I think it's at least as good to offer the fix in this case, and certainly it's not a good reason to drop support for others..

---

> void f1(int &a);

I can't follow this example, there are no macros?
Why would the insertion change semantics?

---

> maybe we should be more conservative? just whitelist some diagnostics? fixing typos seems pretty safe.

I think this leaves a lot of value on the table - we've been conservative so far.
The problem with whitelists is they're incomplete and outdated (e.g. we have a whitelist for include fixer that's very incomplete, and I haven't managed to get around to fixing it, and neither has anyone else).
So I think we should use this (or a blacklist) only if we can show this plausibly causes real problems.

(To put this another way: by being too aggressive we'll get more feedback, by being more conservative we'll continue to get none)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78338