[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:
> 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.
> 
> 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.

This is definitely true.

As discussed offline, this is mitigated by:
 - stringified expansions are common, but don't really count for this purpose
 - fix will often make sense for all occurrences (we can't detect this), in which case not offering it is **worse**
 - remaining cases where fix makes sense for one case and not others probably aren't that numerous
 - behaviour in this case is to apply the fix and diagnose the resulting error from the other expansion, which isn't that terrible (as far as macro diagnostic experience goes)

Agreed to do it anyway and wait for feedback, I'll document this though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78338