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

[Haojian Wu via Phabricator via cfe-commits]

hokein 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() &&
----------------
sammccall wrote:
> 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)
> 
> 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.

ah, you are right.

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

sorry, the example was incomplete, the case is like

```
int f1(int &a);
#define ABC(x) *x + f1(x);
void f2(int *a) {
  ABC(a); // fixit -> ABC(*a), incorrect for the `*x` in macro body.
}
```

if the macro argument is being used in multiple places of the macro body, it probably leads to problems. I suspect this is common in practice, we should not allow fixit in this case.


>  think this leaves a lot of value on the table - we've been conservative so far.

fair enough.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78338