Refactoring ExtInfo to use a builder-style API to enforce correctness-by-construction

[typesanitizer (Varun Gandhi)]

I'm considering refactoring the ExtInfo types in the codebase to use a builder-style API. More concretely, the change would look something like:

// Present:
ExtInfo:
  - // constructors elided
  - getXYZ : ExtInfo -> XYZ 
  - withXYZ : XYZ -> ExtInfo -> ExtInfo

// Future:
ExtInfoBuilder:
  - // constructors elided
  - getXYZ : ExtInfoBuilder -> XYZ
  - withXYZ : XYZ -> ExtInfoBuilder -> ExtInfoBuilder
  - finish : ExtInfoBuilder -> ExtInfo

ExtInfo:
  - // no public constructors
  - getXYZ : ExtInfo -> XYZ
  - rebuild : ExtInfo -> ExtInfoBuilder // (strawman name)

The builder represents a WIP state. In finish, we check all the fields are in a mutually consistent state (when compiling with assertions enabled). This means that it becomes impossible to safely create an ExtInfo in an invalid state. This would avoid some complexity in the code today:

static bool isEscaping(Type type) {
  if (auto *funcType = type->getAs<AnyFunctionType>()) {
    if (funcType->getExtInfo().getRepresentation() ==
          FunctionTypeRepresentation::CFunctionPointer)
      return false;

    return !funcType->getExtInfo().isNoEscape();
  }
  return false;
}

After this change, we will be able to call funcType->getExtInfo().isNoEscape() directly without needing to check the representation.

This should also enable us to roll out changes for propagating Clang types through SIL easier; right now, I'm playing whack-a-mole trying to find places creating an ExtInfo but not setting the Clang type field properly.

The implementation would involve ExtInfo being a single-element struct containing an ExtInfoBuilder (i.e. it is a newtype wrapper which enforces some extra invariants on top of the internal type).

There are a couple of shortcomings with this:

• It is a bit less ergonomic to use for small changes, now you need rebuild().withXYZ().finish() instead of withXYZ().
• There is a little bit of duplication with the getters.

IMO these are acceptable.

Do people have any major concerns with this approach? It might create some merge conflicts in the short term but I think this should be overall a positive for correctness and making future changes easier.

cc @gribozavr, @dan-zheng, @Michael_Gottesman

[David_Ungar2 (David Ungar)]

I've run into something similar, programming in Swift, and have wondered if there's something missing in the language; it's a common pattern with no easy reification I can think of in the language.

Reifying the distinction between under-construction and finished with a type seems right to me. One thought, since you use the name finish, UnfinishedExtInfo a better name than ExtInfoBuilder? Or does the proposed ExtInfoBuilder do more than just set state?

Sorta reminds me of a painting: There's a time when the artist is still applying paint, then a time when the artist is finished. So, "unfinished painting" vs "painting". Just a thought.

Regardless of what you name it, seems like a good thing to do to me.

[typesanitizer (Varun Gandhi)]

@David_Ungar2

Reifying the distinction between under-construction and finished with a type seems right to me. One thought, since you use the name finish , UnfinishedExtInfo a better name than ExtInfoBuilder ? Or does the proposed ExtInfoBuilder do more than just set state?

Hmm. I used Builder in the name because I've seen the phrase "Builder pattern" being used in many places (1, 2), so I figured it would easier for other people to pattern-match and understand what was going on when they saw "Builder" in the name of the type. We could name the final method build instead of finish to match up with Builder.

[typesanitizer (Varun Gandhi)]

I have a draft PR with an implementation of the suggested changes and fixing the resulting fallout: https://github.com/apple/swift/pull/33118

[dfsweeney (Daniel Sweeney)]

A couple of notes from the pull request (it looks good but I have not completely analyzed it yet.):

For .intoBuilder(), does it make sense to instead add the .with_x_ methods directly to the ExtInfo itself, but returning an ExtInfoBuilder? I think that's still typesafe, because you can't directly use the result because it's not the original type. I might not have thought of a case for that though.

It would simplify the syntax but would put the .with_x_ methods back on the ExtInfo. That might be what we are trying to avoid. Possibly using something like .update or .builder or .updater instead of .intoBuilder for the get-a-bulider method might be clearer. I'm not sure if that's possible because ExtInfo is in TypeBase and I haven't gone through the whole thing yet.

Exposing the name of the builder pattern is good because people need to be able to see that it's a builder, so they know that there's a different type until they call .build. But ideally you would not show the name of the implementing pattern in the name of the thing if you could avoid it. I don't know if we can avoid it here because naive clients might not understand it.

It might be clearer to use .update() to return the build object when the object is based on a prototype object with updated fields. In real terms it's a newly constructed object but from the perspective of the coder it's based on something else. I'm not sure if this is more than a synonym for .build() though, so it might be bad to have two identical methods. I might be overthinking that--I was thinking that newly constructed objects have a different feel than updated objects. Using .build for an updated object feels a little weird.

I guess the name ExtInfo can't be made more specific because it's in TypeBase, right? I sort of don't like the name because it does not say what it is (other than being extra or extended information) but the things in the ExtInfo look like they have things in common that could get a more specific name. That's not related to the builder pattern though, I digress. Like I said I have not completely worked through this.

[typesanitizer (Varun Gandhi)]

@dfsweeney

For .intoBuilder() , does it make sense to instead add the .with_x_ methods directly to the ExtInfo itself, but returning an ExtInfoBuilder ? I think that's still typesafe, because you can't directly use the result because it's not the original type. I might not have thought of a case for that though.

Looking at the amount of churn, especially around places setting withNoEscape, I'm considering adding those. The PR is very much in prototype state, not the final API.

@dfsweeney

It would simplify the syntax but would put the .with_x_ methods back on the ExtInfo . That might be what we are trying to avoid.

That would be fine, so long as each method still checks invariants on the result before returning. The point is to have invariants being checked on every code path of construction.

@dfsweeney

Possibly using something like .update or .builder or .updater instead of .intoBuilder for the get-a-bulider method might be clearer.

update hints that some modification might be happening, but the method is const. I could've used builder but that sounded too close to the dual build method.

@dfsweeney

I'm not sure if that's possible because ExtInfo is in TypeBase and I haven't gone through the whole thing yet.

That shouldn't matter. If you look at the PR, I've actually moved the individual types to the top-level (the TLDR is this is needed because of a shared internal type ClangTypeInfo that is friend-ed and the inability to forward-declare nested types). So the PR now has swift::ASTExtInfo, swift::ASTExtInfoBuilder, swift::SILExtInfo and swift::SILExtInfoBuilder, instead of swift::AnyFunctionType::ExtInfo etc.

@dfsweeney

Exposing the name of the builder pattern is good because people need to be able to see that it's a builder, so they know that there's a different type until they call .build . But ideally you would not show the name of the implementing pattern in the name of the thing if you could avoid it. I don't know if we can avoid it here because naive clients might not understand it.

I'm afraid I don't understand this concern. We use the visitor pattern in many places, and the methods are called visitXYZ. To be clear, I'm not against adding withXYZ methods on the ExtInfo types to make things more ergonomic for call-sites which are only updating one field.

@dfsweeney

It might be clearer to use .update() to return the build object when the object is based on a prototype object with updated fields. In real terms it's a newly constructed object but from the perspective of the coder it's based on something else. I'm not sure if this is more than a synonym for .build() though, so it might be bad to have two identical methods. I might be overthinking that--I was thinking that newly constructed objects have a different feel than updated objects. Using .build for an updated object feels a little weird.

The idiom the code has been following so far is that ExtInfo values are immutable and you create slightly differing versions of it. The correct mental model is that new objects are being returned, it is not the case that the existing object is being mutated. So indeed, we have a newly constructed object, not an updated object.

@dfsweeney

I guess the name ExtInfo can't be made more specific because it's in TypeBase , right? I sort of don't like the name because it does not say what it is (other than being extra or extended information) but the things in the ExtInfo look like they have things in common that could get a more specific name. That's not related to the builder pattern though, I digress. Like I said I have not completely worked through this.

Doesn't matter where it is, we can always rename stuff. Personally, I would've preferred if it were named CallingConvInfo or something, but I don't want to push for that change right now (also, [I think] escaping information is not part of the calling convention, so even that would not be technically correct). More generally, yeah it would be a digression to discuss how that type is (or should be) structured and named. Let's not go there.

[dfsweeney (Daniel Sweeney)]

I agree with you about all of the above. I was overthinking it.

The builder pattern bothers me a little because it's achieving the equivalent of mutating an object but actually creates a new one. I think that's where I'm getting some of the crosstalk when I think about it.

For the name of the builder pattern in the name of the method, I was thinking that the method should say what calling it achieves, not how it does it or how it's implemented. You are right that the same criticism applies to visitor.

Anybody who's using the method does need to know how it works, so you do need to indicate that it's a builder. I think this is another thing that bothers me about builder (a little)--it requires the caller to follow some rules that are different from the normal rules. That's OK, I mean a lot of things do that, but it feels a little off to me. Again, overthinking it.