[libTooling] Change `addInclude` to use expansion locs.

[ymandel]

Summary

This patch changes the default range used to anchor the include insertion to use
an expansion loc. This ensures that the location is valid, when the user relies
on the default range.

Driveby: extend a FIXME for a problem that was emphasized by this change; fix some spellings.

[ymandel]

Gentle ping...

clang/include/clang/Tooling/Transformer/RewriteRule.h

 ASTEdit remove(RangeSelector S);
 
 /// Adds an include directive for the given header to the file of `Target`. The
 /// particular location specified by `Target` is ignored.
 ASTEdit addInclude(RangeSelector Target, StringRef Header,
                    IncludeFormat Format = IncludeFormat::Quoted);
 
 /// Adds an include directive for the given header to the file associated with
-/// `RootID`.
+/// `RootID`. If `RootID` matches inside a macro expansion, will add the
+/// directive to the file in which the macro was expanded (as opposed to the
+/// file in which the macro is defined).
 inline ASTEdit addInclude(StringRef Header,
                           IncludeFormat Format = IncludeFormat::Quoted) {
-  return addInclude(node(RootID), Header, Format);
+  return addInclude(expansion(node(RootID)), Header, Format);
 }
 
 // FIXME: If `Metadata` returns an `llvm::Expected<T>` the `AnyGenerator` will
 // construct an `llvm::Expected<llvm::Any>` where no error is present but the
 // `llvm::Any` holds the error. This is unlikely but potentially surprising.
 // Perhaps the `llvm::Expected` should be unwrapped, or perhaps this should be a
 // compile-time error. No solution here is perfect.
 //

 
 /// For every case in Rule, adds an include directive for the given header. The
 /// common use is assumed to be a rule with only one case. For example, to
 /// replace a function call and add headers corresponding to the new code, one
 /// could write:
 /// \code
 ///   auto R = makeRule(callExpr(callee(functionDecl(hasName("foo")))),
 ///            changeTo(cat("bar()")));
-///   AddInclude(R, "path/to/bar_header.h");
-///   AddInclude(R, "vector", IncludeFormat::Angled);
+///   addInclude(R, "path/to/bar_header.h");
+///   addInclude(R, "vector", IncludeFormat::Angled);
 /// \endcode
 void addInclude(RewriteRule &Rule, llvm::StringRef Header,
                 IncludeFormat Format = IncludeFormat::Quoted);
 
 /// Applies the first rule whose pattern matches; other rules are ignored.  If
 /// the matchers are independent then order doesn't matter. In that case,
 /// `applyFirst` is simply joining the set of rules into one.
 //

clang/lib/Tooling/Transformer/RewriteRule.cpp

translateEdits(const MatchResult &Result, ArrayRef<ASTEdit> ASTEdits) {
   SmallVector<transformer::Edit, 1> Edits;
   for (const auto &E : ASTEdits) {
     Expected<CharSourceRange> Range = E.TargetRange(Result);
     if (!Range)
       return Range.takeError();
     llvm::Optional<CharSourceRange> EditRange =
         tooling::getRangeForEdit(*Range, *Result.Context);
     // FIXME: let user specify whether to treat this case as an error or ignore
-    // it as is currently done.
+    // it as is currently done. This behavior is problematic in that it hides
+    // failures from bad ranges. Also, the behavior here differs from
+    // `flatten`. Here, we abort (without error), whereas flatten, if it hits an
+    // empty list, does not abort. As a result, `editList({A,B})` is not
+    // equivalent to `flatten(edit(A), edit(B))`. The former will abort if `A`
+    // produces a bad range, whereas the latter will simply ignore A.
     if (!EditRange)
       return SmallVector<Edit, 0>();
     auto Replacement = E.Replacement->eval(Result);
     if (!Replacement)
       return Replacement.takeError();
     auto Metadata = E.Metadata(Result);
     if (!Metadata)
       return Metadata.takeError();