Merge pull request swiftlang#29412 from brentdax/two-revisions-for-slashing

[SE-0274] Update concise #file implementation in response to first round of review

Author: beccadax

include/swift/AST/DiagnosticsCommon.def

@@ -139,6 +139,14 @@ ERROR(sdk_node_unrecognized_decl_kind,none,
 ERROR(sdk_node_unrecognized_accessor_kind,none,
       "unrecognized accessor kind '%0' in SDK node", (StringRef))
 
+// Emitted from ModuleDecl::computeMagicFileStringMap()
+WARNING(pound_source_location_creates_pound_file_conflicts,none,
+        "'#sourceLocation' directive produces '#file' string of '%0', which "
+        "conflicts with '#file' strings produced by other paths in the module",
+        (StringRef))
+NOTE(fixit_correct_source_location_file,none,
+     "change file in '#sourceLocation' to '%0'", (StringRef))
+
 //------------------------------------------------------------------------------
 // MARK: Circular reference diagnostics
 //------------------------------------------------------------------------------

include/swift/AST/Module.h

@@ -109,6 +109,38 @@ enum class SourceFileKind {
   Interface ///< Came from a .swiftinterface file, representing another module.
 };
 
+/// Contains information about where a particular path is used in
+/// \c SourceFiles.
+struct SourceFilePathInfo {
+  struct Comparator {
+    bool operator () (SourceLoc lhs, SourceLoc rhs) const {
+      return lhs.getOpaquePointerValue() <
+             rhs.getOpaquePointerValue();
+    }
+  };
+
+  SourceLoc physicalFileLoc{};
+  std::set<SourceLoc, Comparator> virtualFileLocs{}; // std::set for sorting
+
+  SourceFilePathInfo() = default;
+
+  void merge(const SourceFilePathInfo &other) {
+    if (other.physicalFileLoc.isValid()) {
+      assert(!physicalFileLoc.isValid());
+      physicalFileLoc = other.physicalFileLoc;
+    }
+
+    for (auto &elem : other.virtualFileLocs) {
+      virtualFileLocs.insert(elem);
+    }
+  }
+
+  bool operator == (const SourceFilePathInfo &other) const {
+    return physicalFileLoc == other.physicalFileLoc &&
+           virtualFileLocs == other.virtualFileLocs;
+  }
+};
+
 /// Discriminator for resilience strategy.
 enum class ResilienceStrategy : unsigned {
   /// Public nominal types: fragile
@@ -257,6 +289,24 @@ class ModuleDecl : public DeclContext, public TypeDecl {
   void addFile(FileUnit &newFile);
   void removeFile(FileUnit &existingFile);
 
+  /// Creates a map from \c #filePath strings to corresponding \c #file
+  /// strings, diagnosing any conflicts.
+  ///
+  /// A given \c #filePath string always maps to exactly one \c #file string,
+  /// but it is possible for \c #sourceLocation directives to introduce
+  /// duplicates in the opposite direction. If there are such conflicts, this
+  /// method will diagnose the conflict and choose a "winner" among the paths
+  /// in a reproducible way. The \c bool paired with the \c #file string is
+  /// \c true for paths which did not have a conflict or won a conflict, and
+  /// \c false for paths which lost a conflict. Thus, if you want to generate a
+  /// reverse mapping, you should drop or special-case the \c #file strings that
+  /// are paired with \c false.
+  ///
+  /// Note that this returns an empty StringMap if concise \c #file strings are
+  /// disabled. Users should fall back to using the file path in this case.
+  llvm::StringMap<std::pair<std::string, /*isWinner=*/bool>>
+  computeMagicFileStringMap(bool shouldDiagnose) const;
+
   /// Add a file declaring a cross-import overlay.
   void addCrossImportOverlayFile(StringRef file);
 

include/swift/AST/SourceFile.h

@@ -321,6 +321,12 @@ class SourceFile final : public FileUnit {
   /// forwarded on to IRGen.
   ASTStage_t ASTStage = Unprocessed;
 
+  /// Virtual filenames declared by #sourceLocation(file:) directives in this
+  /// file.
+  llvm::SmallVector<Located<StringRef>, 0> VirtualFilenames;
+
+  llvm::StringMap<SourceFilePathInfo> getInfoForUsedFilePaths() const;
+
   SourceFile(ModuleDecl &M, SourceFileKind K, Optional<unsigned> bufferID,
              ImplicitModuleImportKind ModImpKind, bool KeepParsedTokens = false,
              bool KeepSyntaxTree = false, ParsingOptions parsingOpts = {});

lib/AST/Module.cpp

@@ -1955,6 +1955,144 @@ static void performAutoImport(
   SF.addImports(Imports);
 }
 
+llvm::StringMap<SourceFilePathInfo>
+SourceFile::getInfoForUsedFilePaths() const {
+  llvm::StringMap<SourceFilePathInfo> result;
+
+  if (BufferID != -1) {
+    result[getFilename()].physicalFileLoc =
+        getASTContext().SourceMgr.getLocForBufferStart(BufferID);
+  }
+
+  for (auto &vpath : VirtualFilenames) {
+    result[vpath.Item].virtualFileLocs.insert(vpath.Loc);
+  }
+
+  return result;
+}
+
+/// Returns a map of filenames to a map of file paths to SourceFilePathInfo
+/// instances, for all SourceFiles in the module.
+static llvm::StringMap<llvm::StringMap<SourceFilePathInfo>>
+getInfoForUsedFileNames(const ModuleDecl *module) {
+  llvm::StringMap<llvm::StringMap<SourceFilePathInfo>> result;
+
+  for (auto *file : module->getFiles()) {
+    auto *sourceFile = dyn_cast<SourceFile>(file);
+    if (!sourceFile) continue;
+
+    for (auto &pair : sourceFile->getInfoForUsedFilePaths()) {
+      StringRef fullPath = pair.first();
+      StringRef fileName = llvm::sys::path::filename(fullPath);
+      auto &info = pair.second;
+
+      result[fileName][fullPath].merge(info);
+    }
+  }
+
+  return result;
+}
+
+static void
+computeMagicFileString(const ModuleDecl *module, StringRef name,
+                       SmallVectorImpl<char> &result) {
+  result.assign(module->getNameStr().begin(), module->getNameStr().end());
+  result.push_back('/');
+  result.append(name.begin(), name.end());
+}
+
+static StringRef
+resolveMagicNameConflicts(const ModuleDecl *module, StringRef fileString,
+                          const llvm::StringMap<SourceFilePathInfo> &paths,
+                          bool shouldDiagnose) {
+  assert(paths.size() > 1);
+  assert(module->getASTContext().LangOpts.EnableConcisePoundFile);
+
+  /// The path we consider to be "correct"; we will emit fix-its changing the
+  /// other paths to match this one.
+  StringRef winner = "";
+
+  // First, select a winner.
+  for (const auto &pathPair : paths) {
+    // If there is a physical file with this name, we use its path and stop
+    // looking.
+    if (pathPair.second.physicalFileLoc.isValid()) {
+      winner = pathPair.first();
+      break;
+    }
+
+    // Otherwise, we favor the lexicographically "smaller" path.
+    if (winner.empty() || winner > pathPair.first()) {
+      winner = pathPair.first();
+    }
+  }
+
+  // If we're not diagnosing, that's all we need to do.
+  if (!shouldDiagnose)
+    return winner;
+
+  SmallString<64> winnerLiteral;
+  llvm::raw_svector_ostream winnerLiteralStream{winnerLiteral};
+  swift::printAsQuotedString(winnerLiteralStream, winner);
+
+  auto &diags = module->getASTContext().Diags;
+
+  // Diagnose the conflict at each #sourceLocation that specifies it.
+  for (const auto &pathPair : paths) {
+    bool isWinner = (pathPair.first() == winner);
+
+    // Don't diagnose #sourceLocations that match the physical file.
+    if (pathPair.second.physicalFileLoc.isValid()) {
+      assert(isWinner && "physical files should always win; duplicate name?");
+      continue;
+    }
+
+    for (auto loc : pathPair.second.virtualFileLocs) {
+      diags.diagnose(loc,
+                     diag::pound_source_location_creates_pound_file_conflicts,
+                     fileString);
+
+      // Offer a fix-it unless it would be tautological.
+      if (!isWinner)
+        diags.diagnose(loc, diag::fixit_correct_source_location_file, winner)
+          .fixItReplace(loc, winnerLiteral);
+    }
+  }
+
+  return winner;
+}
+
+llvm::StringMap<std::pair<std::string, bool>>
+ModuleDecl::computeMagicFileStringMap(bool shouldDiagnose) const {
+  llvm::StringMap<std::pair<std::string, bool>> result;
+  SmallString<64> scratch;
+
+  if (!getASTContext().LangOpts.EnableConcisePoundFile)
+    return result;
+
+  for (auto &namePair : getInfoForUsedFileNames(this)) {
+    computeMagicFileString(this, namePair.first(), scratch);
+    auto &infoForPaths = namePair.second;
+
+    assert(!infoForPaths.empty());
+
+    // TODO: In the future, we'd like to handle these conflicts gracefully by
+    // generating a unique `#file` string for each conflicting name. For now, we
+    // will simply warn about conflicts.
+    StringRef winner = infoForPaths.begin()->first();
+    if (infoForPaths.size() > 1)
+      winner = resolveMagicNameConflicts(this, scratch, infoForPaths,
+                                         shouldDiagnose);
+
+    for (auto &pathPair : infoForPaths) {
+      result[pathPair.first()] =
+          std::make_pair(scratch.str().str(), pathPair.first() == winner);
+    }
+  }
+
+  return result;
+}
+
 SourceFile::SourceFile(ModuleDecl &M, SourceFileKind K,
                        Optional<unsigned> bufferID,
                        ImplicitModuleImportKind ModImpKind,

lib/Parse/ParseDecl.cpp

@@ -4822,7 +4822,8 @@ ParserStatus Parser::parseLineDirective(bool isLine) {
           getStringLiteralIfNotInterpolated(Loc, "'#sourceLocation'");
       if (!Filename.hasValue())
         return makeParserError();
-      consumeToken(tok::string_literal);
+      SourceLoc filenameLoc = consumeToken(tok::string_literal);
+      SF.VirtualFilenames.emplace_back(*Filename, filenameLoc);
 
       if (parseToken(tok::comma, diag::sourceLocation_expected, ",") ||
           parseSpecificIdentifier("line", diag::sourceLocation_expected,

lib/SIL/SILPrinter.cpp

@@ -47,6 +47,7 @@
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/FileSystem.h"
+#include <set>
 
 
 using namespace swift;
@@ -2750,6 +2751,57 @@ printSILCoverageMaps(SILPrintContext &Ctx,
     M->print(Ctx);
 }
 
+using MagicFileStringMap =
+    llvm::StringMap<std::pair<std::string, /*isWinner=*/bool>>;
+
+static void
+printMagicFileStringMapEntry(SILPrintContext &Ctx,
+                             const MagicFileStringMap::MapEntryTy &entry) {
+  auto &OS = Ctx.OS();
+  OS << "//   '" << std::get<0>(entry.second)
+     << "' => '" << entry.first() << "'";
+
+  if (!std::get<1>(entry.second))
+    OS << " (alternate)";
+
+  OS << "\n";
+}
+
+static void printMagicFileStringMap(SILPrintContext &Ctx,
+                                    const MagicFileStringMap map) {
+  if (map.empty())
+    return;
+
+  Ctx.OS() << "\n\n// Mappings from '#file' to '#filePath':\n";
+
+  if (Ctx.sortSIL()) {
+    llvm::SmallVector<llvm::StringRef, 16> keys;
+    llvm::copy(map.keys(), std::back_inserter(keys));
+
+    llvm::sort(keys, [&](StringRef leftKey, StringRef rightKey) -> bool {
+      const auto &leftValue = map.find(leftKey)->second;
+      const auto &rightValue = map.find(rightKey)->second;
+
+      // Lexicographically earlier #file strings sort earlier.
+      if (std::get<0>(leftValue) != std::get<0>(rightValue))
+        return std::get<0>(leftValue) < std::get<0>(rightValue);
+
+      // Conflict winners sort before losers.
+      if (std::get<1>(leftValue) != std::get<1>(rightValue))
+        return std::get<1>(leftValue);
+
+      // Finally, lexicographically earlier #filePath strings sort earlier.
+      return leftKey < rightKey;
+    });
+
+    for (auto key : keys)
+      printMagicFileStringMapEntry(Ctx, *map.find(key));
+  } else {
+    for (const auto &entry : map)
+      printMagicFileStringMapEntry(Ctx, entry);
+  }
+}
+
 void SILProperty::print(SILPrintContext &Ctx) const {
   PrintOptions Options = PrintOptions::printSIL();
 
@@ -2864,6 +2916,10 @@ void SILModule::print(SILPrintContext &PrintCtx, ModuleDecl *M,
   printSILProperties(PrintCtx, getPropertyList());
   printExternallyVisibleDecls(PrintCtx, externallyVisible.getArrayRef());
 
+  if (M)
+    printMagicFileStringMap(
+        PrintCtx, M->computeMagicFileStringMap(/*shouldDiagnose=*/false));
+
   OS << "\n\n";
 }
 

lib/SILGen/SILGen.cpp

@@ -50,7 +50,9 @@ using namespace Lowering;
 //===----------------------------------------------------------------------===//
 
 SILGenModule::SILGenModule(SILModule &M, ModuleDecl *SM)
-    : M(M), Types(M.Types), SwiftModule(SM), TopLevelSGF(nullptr) {
+    : M(M), Types(M.Types), SwiftModule(SM), TopLevelSGF(nullptr),
+      MagicFileStringsByFilePath(
+          SM->computeMagicFileStringMap(/*shouldDiagnose=*/true)) {
   const SILOptions &Opts = M.getOptions();
   if (!Opts.UseProfile.empty()) {
     auto ReaderOrErr = llvm::IndexedInstrProfReader::create(Opts.UseProfile);

lib/SILGen/SILGen.h

@@ -131,6 +131,9 @@ class LLVM_LIBRARY_VISIBILITY SILGenModule : public ASTVisitor<SILGenModule> {
 
   ASTContext &getASTContext() { return M.getASTContext(); }
 
+  llvm::StringMap<std::pair<std::string, /*isWinner=*/bool>>
+  MagicFileStringsByFilePath;
+
   static DeclName getMagicFunctionName(SILDeclRef ref);
   static DeclName getMagicFunctionName(DeclContext *dc);
 

lib/SILGen/SILGenApply.cpp

@@ -4892,14 +4892,11 @@ StringRef SILGenFunction::getMagicFilePathString(SourceLoc loc) {
 std::string SILGenFunction::getMagicFileString(SourceLoc loc) {
   auto path = getMagicFilePathString(loc);
 
-  if (!getASTContext().LangOpts.EnableConcisePoundFile)
-    return path;
+  auto result = SGM.MagicFileStringsByFilePath.find(path);
+  if (result != SGM.MagicFileStringsByFilePath.end())
+    return std::get<0>(result->second);
 
-  auto value = llvm::sys::path::filename(path).str();
-  value += " (";
-  value += getModule().getSwiftModule()->getNameStr();
-  value += ")";
-  return value;
+  return path;
 }
 
 /// Emit an application of the given allocating initializer.

test/SILGen/Inputs/magic_identifier_file_conflicting_other.swift

@@ -0,0 +1,8 @@
+// This file matches test/SILGen/magic_identifier_file_conflicting.swift.
+// It should be compiled with -verify.
+
+// We should diagnose cross-file conflicts.
+// expected-warning@+2 {{'#sourceLocation' directive produces '#file' string of 'Foo/other_file_c.swift', which conflicts with '#file' strings produced by other paths in the module}}
+// expected-note@+1 {{change file in '#sourceLocation' to 'first/other_file_c.swift'}} {{23-50="first/other_file_c.swift"}}
+#sourceLocation(file: "second/other_file_c.swift", line: 1)
+#sourceLocation()