[clang] Remove misleading assertion in FullSourceLoc

[jansvoboda11]

Summary

D31709 added an assertion was added to FullSourceLoc::hasManager() that ensured a valid SourceLocation is always paired with a SourceManager, and missing SourceManager is always paired with an invalid SourceLocation.

This appears to be incorrect, since clients never cared about constructing FullSourceLoc to uphold that invariant, or always checking isValid() before calling hasManager().

The assertion started failing when serializing diagnostics pointing into an explicit module. Explicit modules don't have valid SourceLocation for the import statement, since they are "imported" from the command-line argument -fmodule-name=x.pcm.

This patch removes the assertion, since FullSourceLoc was never intended to uphold any kind of invariants between the validity of SourceLocation and presence of SourceManager.

[arphaman]

I think this approach makes sense for now. It's unfortunate that the constructor of FullSourceLoc is unable to validate this requirement, do you know how many clients that you describe as modifying their ID to make them valid are there?

[dexonsmith]

In D106862#2913815, @arphaman wrote:

I think this approach makes sense for now. It's unfortunate that the constructor of FullSourceLoc is unable to validate this requirement, do you know how many clients that you describe as modifying their ID to make them valid are there?

(Maybe an Optional<FullSourceLoc> would work for those clients?)

[jansvoboda11]

Remove assertion altogether.

[jansvoboda11]

I investigated this a bit more.

What previously lead me to believe that SourceLoc::ID is being modified from the outside was the way it's declared right after some friends:

class SourceLocation {
  friend class ASTReader;
  friend class ASTWriter;
  friend class SourceManager;
  friend struct llvm::FoldingSetTrait<SourceLocation>;

public:
  using UIntTy = uint32_t;
  using IntTy = int32_t;

private:
  UIntTy ID = 0;

I looked up the actual writes to ID and they only occur in the SourceLocation factory functions, for example:

SourceLocation getLocWithOffset(IntTy Offset) const {
  assert(((getOffset()+Offset) & MacroIDBit) == 0 && "offset overflow");
  SourceLocation L;
  L.ID = ID+Offset;
  return L;
}

Theoretically there's nothing preventing the friends from modifying the field as well, but that's not being abused at this moment.

This means that if the FullSourceLoc(SourceLocation, SourceManager &) constructor was to be called with an invalid SourceLocation, it would stay invalid for the whole lifetime of FullSourceLoc.

---

So if we really expect FullSourceLoc to always uphold this invariant: (SrcMgr != nullptr) == isValid(), we should be able to replace this assertion:

bool hasManager() const {
    bool hasSrcMgr =  SrcMgr != nullptr;
    assert(hasSrcMgr == isValid() && "FullSourceLoc has location but no manager");
    return hasSrcMgr;
}

with this one:

explicit FullSourceLoc(SourceLocation Loc, const SourceManager &SM)
    : SourceLocation(Loc), SrcMgr(&SM) {
  assert(isValid() && "FullSourceLoc has manager but no location");
}

That way, we can catch any future misuse of FullSourceLoc much earlier.

---

But: I think the intention for FullSourceLoc has never been to uphold that invariant:

• FullSourceLoc is being constructed without clients ensuring the validity of the SourceLoc.
  • Moving the assertion into the constructor and running tests confirmed FullSourceLoc is indeed being constructed with invalid SourceLocation (and valid SourceManager).
• The clients also never check isValid() before calling hasManager() to avoid the assertion in hasManager(), which is odd.
  • I think the assertion in hasManager has never been hit before simply by pure luck.

---

Considering that FullSourceLoc has been around since 2007 and the assertion was just added in 2017, I think the correct thing to do here is to remove the assertion and properly document that FullSourceLoc does not uphold any invariants.

[jansvoboda11]

Ping.

clang/include/clang/Basic/SourceLocation.h

public:
   }
 };
 
 class FileEntry;
 
 /// A SourceLocation and its associated SourceManager.
 ///
 /// This is useful for argument passing to functions that expect both objects.
+///
+/// This class does not guarantee the presence of either the SourceManager or
+/// a valid SourceLocation. Clients should use `isValid()` and `hasManager()`
+/// before calling the member functions.
 class FullSourceLoc : public SourceLocation {
   const SourceManager *SrcMgr = nullptr;
 
 public:
   /// Creates a FullSourceLoc where isValid() returns \c false.
   FullSourceLoc() = default;
 
   explicit FullSourceLoc(SourceLocation Loc, const SourceManager &SM)
       : SourceLocation(Loc), SrcMgr(&SM) {}
 
-  bool hasManager() const {
-      bool hasSrcMgr =  SrcMgr != nullptr;
-      assert(hasSrcMgr == isValid() && "FullSourceLoc has location but no manager");
-      return hasSrcMgr;
-  }
+  /// Checks whether the SourceManager is present.
+  bool hasManager() const { return SrcMgr != nullptr; }
 
-  /// \pre This FullSourceLoc has an associated SourceManager.
+  /// \pre hasManager()
   const SourceManager &getManager() const {
     assert(SrcMgr && "SourceManager is NULL.");
     return *SrcMgr;
   }
 
   FileID getFileID() const;
 
   FullSourceLoc getExpansionLoc() const;

clang/test/Modules/Inputs/explicit-build-diags/a.h

+void a() __attribute__((deprecated));

clang/test/Modules/Inputs/explicit-build-diags/module.modulemap

+module a { header "a.h" }

clang/test/Modules/explicit-build-diags.cpp

+// RUN: rm -rf %t && mkdir %t
+// RUN: %clang_cc1 -fmodules -x c++ %S/Inputs/explicit-build-diags/module.modulemap -fmodule-name=a -emit-module -o %t/a.pcm
+// RUN: %clang_cc1 -fmodules -Wdeprecated-declarations -fdiagnostics-show-note-include-stack -serialize-diagnostic-file %t/tu.dia \
+// RUN:   -I %S/Inputs/explicit-build-diags -fmodule-file=%t/a.pcm -fsyntax-only %s
+
+#include "a.h"
+
+void foo() { a(); }