A possible gotcha in overload resolution for PersistableRecord

[mbrandonw]

Due to how Swift resolves overloads, there is the potential for a gotcha when using insert(db). Consider the following record type:

struct Player: MutablePersistableRecord, Encodable {
  var id: Int64?
  mutating func didInsert(_ inserted: InsertionSuccess) {
    id = inserted.rowID
  }
}

Note that didInsert is provided in order to update the id after insertion.

With that it would be reasonable to write code like this:

var player = Player(…)
try player.insert(db)
_ = player.id!

This works just fine and we can be assured that id is safe to force unwrap because of the didInsert.

The problem is if someday in the future we decide that we want the functionality of PersistableRecord and so we upgrade our Player type:

struct Player: PersistableRecord, Encodable {
  …
}

We have unfortunately now created a problem everywhere we are doing player.insert and would need to audit all usages. The problem is that player.insert now resolves to the non-mutating version of insert, and through that version the callbacks are not invoked and the ID is not updated:

_ = player.id!  // 💥 Crash

Even though the player is a var, Swift is still preferring the overload on PersistableRecord over MutablePersistableRecord because it is more specific.

Here is a toy example of how this resolution problem manifests:

protocol MutableP {}
extension MutableP {
  mutating func foo() {}
}
protocol P: MutableP {}
extension P {
  func foo() {}
}
extension Int: P {}
func foo() {
  let x = 1; x.foo()
  var y = 1; y.foo()
}

Both x.foo and y.foo resolve to the foo defined on P. One might hope that y.foo resolves to MutableP's foo since y is mutable, but that's not how the resolution works.

If this is thought to be a legitimate problem, then one possible fix is to just mark all of the non-mutating methods as @_disfavoredOverload. Here is the fix for the toy example above:

protocol MutableP {}
extension MutableP {
  mutating func foo() {}
}
protocol P: MutableP {}
extension P {
  @_disfavoredOverload
  func foo() {}
}
extension Int: P {}
func foo() {
  let x = 1; x.foo()
  var y = 1; y.foo()
}

Now x.foo resolves to the non-mutating method and y.foo resolves to the mutating one since y is mutable. I guess technically this could be a breaking change, and it is debatable whether it is a bug fix or not.

Another path forward, though far more invasive, is to consider whether the protocols should be flipped. Should MutablePersistableRecord inherit from PersistableRecord, much like how MutableCollection inherits from Collection? I am not well-versed enough in the library to know if that makes sense, but it’s just an armchair observation.

[groue]

Hello @mbrandonw,

The problem is if someday in the future we decide that we want the functionality of PersistableRecord and so we upgrade our Player type:

struct Player: PersistableRecord, Encodable {
  …
}

This is an interesting scenario: thanks for discussing about it. Is it possible to make the motivation explicit? What do you mean with "we want the functionality of PersistableRecord"?

It looks like you aim at this behavior:

extension Player: PersistableRecord { }

let player1 = Player(...) // let
try player1.insert(db) // id is nil in this case

var player2 = Player(...) // var
try player2.insert(db) // id is not nil in this case

This is indeed not how the current API works. If you want insertion to set the id, then you opt in with MutablePersistableRecord, and the let version does not compile:

extension Player: MutablePersistableRecord { }

let player1 = Player(...) // let
try player1.insert(db) // does not compile

var player2 = Player(...) // var
try player2.insert(db) // id is never nil

The fact that the above let variant does not compile has an advantage, because it is impossible to guess the user intent. Is the user you, Brandon, or someone who wrote let instead of var by mistake? With the help of the compiler, we current api makes sure that user intent is explicit.

And if you don't want insertion to mutate the record, then you opt in with PersistableRecord, and insertion never mutates the record. No question.

Also consider that the current api makes sure that all defined persistence callbacks are called (not only didInsert). The user opts in for mutating, (x)or non-mutating variants, and does not have to care about later uses of var or let records, and their eventual runtime difference.

With your suggested change, the user has to care as soon as a callback must be called in both mutating and non-mutating scenarios. It is not trivial:

One mutating and one non-mutating callback

protocol MutableP {
    mutating func callback()
}
protocol P: MutableP {
    func callback()
}

struct Convoluted: P {
    @_implements(MutableP, callback())
    mutating func mutatingCallback() {
        print("Convoluted.mutatingCallback")
    }
    
    @_implements(P, callback())
    func nonMutatingCallback() {
        print("Convoluted.nonMutatingCallback")
    }
}

func run() {
    // Convoluted.mutatingCallback
    var x: some MutableP = Convoluted(); x.callback()
    
    // Convoluted.nonMutatingCallback
    let y: some P = Convoluted(); y.callback()
}

run()

It is desirable to drive users into writing this kind of code? Debatable.

Another path forward, though far more invasive, is to consider whether the protocols should be flipped.

This would be unsound. Non-mutating is a subtype of mutating, not the other way around. A generic method that accepts all records must deal with MutablePersistableRecord, and assume insertion may mutate. MutableCollection can only derive from Collection because it does not turn non-mutating methods of Collection into mutating methods.

[mbrandonw]

Hi @groue,

This is an interesting scenario: thanks for discussing about it. Is it possible to make the motivation explicit?

I do not have a concrete real world example to give, but I think it is typically common for one to slowly introduce protocol conformance as we need their functionality. For example, one may start with a data type being Decodable for an API client until the day they need the full blown Codable.

So, one might be tempted to conform to MutablePersistableRecord since it provides the functionality one needs when they have a mutable value to insert:

var player = Player(…)
try player.insert(db)
_ = player.id!

But then later on that find that they often deal with immutable values for which the above pattern doesn't work. And maybe for awhile they do the var player = player dance to work around it, but then they learn about the PersistableRecord protocol that provides a tool to just do:

let player = someExternalAPI()
try player.insert(db)

But by simply moving to a sub-protocol one has inadvertently changed the behavior of .insert(…) when called on mutable variables. I've been trying to think of a protocol hierarchy in the Swift ecosystem where moving from a protocol to one of its sub-protocols changes the behavior of its APIs. I'm not aware of any, and so that is what stood out to me.

I'll keep digging into the APIs in order to see how everything fits together.

[groue]

OK. Learning a new API can be tough.

Please ask more usage questions as they come up to your mind, I'll try to tell how GRDB can fit your use cases.

As a general advice, please allow the database schema to drive the record types. The schema changes, but not that frequently. Auto-incremented ids are almost always better served with MutablePersistableRecord. The doc is not wrong when it says:

• If your type is a class, choose PersistableRecord. On top of that, implement didInsert(_:) if the database table has an auto-incremented primary key.

• If your type is a struct, and the database table has an auto-incremented primary key, choose MutablePersistableRecord, and implement didInsert(_:).

• Otherwise, choose PersistableRecord, and ignore didInsert(_:).

This is not the full end of the story, because there are as many use cases as there are applications. An example is Record Timestamps, where MutablePersistableRecord makes it possible to set the creation date on insertion, even when the id is not auto-incremented. It is good to use record protocols as a way to help applications deal with database behaviors or constraints. They're pretty good at that: Recommended Practices for Designing Record Types is a recommended reading.

[mbrandonw]

Thanks for the link to the docs. If what is written there is a hard rule, then would it make sense to have PersistableRecord be constrained to AnyObject? That would guide people to the intended usage, and it also fixes the gotcha I am describing.

[groue]

For tables whose primary key is not auto-incremented (string ids, join tables in many-to-many associations, etc.) it is not uncommon to define a PersistableRecord struct :-)

// create(table: "player") { t in
//   t.autoIncrementedPrimaryKey("id")
//   ...
// }
struct Player: Codable, FetchableRecord, MutablePersistableRecord { ... }

// create(table: "team") { t in
//   t.primaryKey("id", .text)
//   ...
// }
struct Team: Codable, FetchableRecord, PersistableRecord { ... }

// create(table: "membership") { t in
//   t.primaryKey {
//     t.belongsTo("player", onDelete: .cascade)
//     t.belongsTo("team", onDelete: .cascade)
//   }
//   t.column("role", .text).notNull()
//   ...
// }
struct Membership: Codable, FetchableRecord, PersistableRecord { ... }

The other frequent protocol combination is Decodable, FetchableRecord, for structs that represent a read-only graph built out of associations:

struct TeamInfo: Decodable, FetchableRecord {
    var team: Team
    var playerCount: Int
    var captain: Player
}

Finally, Codable is not strictly necessary and can be avoided when performance is crucial.