[clang-tidy] Fix `TransformerClangTidyCheck`'s handling of include insertions.

[ymandel]

Summary

Currently, all include insertions are directed to the main file. However,
Transformer rules can specify alternative destinations for include
insertions. This patch fixes the code to associate the include with the correct
file.

This patch was tested manually. The clang tidy unit test framework does not
support testing changes to header files. Given that this is a bug fix for a live
bug, the patch relies on manual testing rather than blocking on upgrading the
unit test framework.

[hokein]

Thanks, the fix looks good.

This patch was tested manually. The clang tidy unit test framework does not
support testing changes to header files. Given that this is a bug fix for a live
bug, the patch relies on manual testing rather than blocking on upgrading the
unit test framework.

If extending the test framework is too much work, I think we can write a lit test for a transformer-based check e.g. abseil-string-find-str-contains: setup an include header, and verify the include is inserted in the header.

[alexfh]

clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp

108

Can this be a macro file id? I'd suggest to add tests (probably for checks using this functionality) with a few nested includes and fixes in normal code, code in macros declared and expanded in different files, locations in macro bodies, macro arguments, and some tricky cases like fix pointing to a pasted token.

[ymandel]

clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp

108

Can this be a macro file id? I'd suggest to add tests (probably for checks using this functionality) with a few nested includes and fixes in normal code, code in macros declared and expanded in different files, locations in macro bodies, macro arguments, and some tricky cases like fix pointing to a pasted token.

Unlikely, given that API that populates the location. Most use a default verion which explicitly maps the location to the expansion location. However, it is possible. Also, Transformer rejects changes inside macros (except arguments) so that would also be hard to engineer.

Still, I'll look into better tests in the check itself. I did try that but mostly failed because the lit tests don't support checking anything in headers. However, the macro checks sound worth it.

clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp

void TransformerClangTidyCheck::check(
   // Associate the diagnostic with the location of the first change.
   DiagnosticBuilder Diag = diag((*Edits)[0].Range.getBegin(), *Explanation);
   for (const auto &T : *Edits)
     switch (T.Kind) {
     case transformer::EditKind::Range:
       Diag << FixItHint::CreateReplacement(T.Range, T.Replacement);
       break;
     case transformer::EditKind::AddInclude:
-      Diag << Inserter.createMainFileIncludeInsertion(T.Replacement);
+      Diag << Inserter.createIncludeInsertion(
+          Result.SourceManager->getFileID(T.Range.getBegin()), T.Replacement);
       break;
     }
 }
 
 void TransformerClangTidyCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "IncludeStyle", Inserter.getStyle());
 }
 
 } // namespace utils
 } // namespace tidy
 } // namespace clang