[PATCH] D106862: [clang][modules] Avoid creating partial FullSourceLoc for explicit modules imports

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 requested review of this revision.
jansvoboda11 added a comment.

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 `friend`s:

  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 <https://reviews.llvm.org/D31709?id=95541#inline-279799> `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.


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

https://reviews.llvm.org/D106862