I've noticed we have a lot of comments on functions that say things like "The collection IX lock must be held before calling this function", and then inside the function body, we will invariant that the correct lock is held. For example, see comments for DatabaseHolder::openDb, and the corresponding invariant.

This was true in some sharding code as well, and in the past the comments have gotten stale, and the invariant may or may not be kept up-to-date. (The invariant probably stays up-to-date.)

I'm wondering if we could instead use a construct akin to WithLock, but that works with database/collection locks instead.

So DatabaseHolder::openDb would look more like

    virtual Database* openDb(OperationContext* opCtx,
                                                 const DatabaseName& dbName,
                                                 WithDbLock::X,
                                                 bool* justCreated = nullptr) = 0;

and callers might do something like

dbHolder->openDb(
        opCtx, 
        dbName, 
        WithDbLock::X(opCtx->lockState(), dbName));

The constructor of WithDbLock::X could have an invariant that the correct lock is held for the database. So it's kind of a hybrid of doing some type-checking as well as some runtime checks (which I think are required since you want to check that the lock is held for the correct database).

You could also have constructors of WithDbLock::X (and the other variants for other lock types) also be implicitly convertible from DBLock so that use cases like this one could pass the lock object directly, like:

Lock::DBLock dbLock(opCtx, dbName, MODE_X);
dbHolder->openDb(
        opCtx, 
        dbName, 
        dbLock);

To do this properly so that you catch invalid lock types at compile time might require some extra work in making different DBLock types like DBLock::X, DBLock::IX that are usable instead of passing flags. E.g.:

Lock::DBLock::X dbLock(opCtx, dbName);
dbHolder->openDb(
        opCtx, 
        dbName, 
        dbLock);

Then you could have the correct lock types be implicitly convertible from others so that the semantics worked correctly. E.g. a WithDbLock::IX would be constructible from a DBLock::X and a DBLock::IX, but not from DBLock::S.

One downside is I'm not sure how to enforce that the caller would pass in the name for the correct database, but that error feels hard to make.

I'm not sure whether on the whole this would prevent bugs or not, but it might help a little with documentation at the very least.