Require SwiftASTContextReader to be valid at all times

[vedantk]

Prior to this patch, it was possible to construct an invalid
SwiftASTContextReader (e.g. via default-init). This led to subtle bugs:
in rdar://65276251 for example, an invalid SwiftASTContextReader was
assigned into an llvm::Optional, and subsequent code crashed because it
assumed that the loaded llvm::Optional contained a valid instance.

Make it so that SwiftASTContextReader is valid at all times.

rdar://65276251

[vedantk]

@swift-ci test

[adrian-prantl]

I guess we could leave this as is, too.

[adrian-prantl]

It's not quite clear to me how this change improves the code, but I'm probably missing something. Previously SwiftASTContextReader had an operator bool() that returned false if the context was invalid, now we vend an Optional, with effectively the same semantics?

What's an example of a bad use the this prevents?

[adrian-prantl]

Looking at the radar, could we have fixed this by changing

   llvm::Optional<SwiftASTContextReader> scratch_ctx;
   if (instance) {
    scratch_ctx = instance->GetScratchSwiftASTContext();
    if (!scratch_ctx)
       return llvm::None;
   }

to

   SwiftASTContextReader scratch_ctx;
   if (instance) {
    scratch_ctx = instance->GetScratchSwiftASTContext();
    if (!scratch_ctx)
       return llvm::None;
   }

?

[adrian-prantl]

My point is, I don't think we can prevent that kind of stupidity at the call site ;-)
(speaking as the likely author here)

[adrian-prantl]

Hmm, you are right, this patch does protect against this misuse, because assigning an Optional to an Optional won't auto-wrap the Optional in another Optional!

[vedantk]

Yep, we certainly could have fixed the bug that way.

In general I think Optional<T> x = get(); if (!x) { ... } is much better than T x = get(); if (!x) { ... }. For one, it forces us to think about validity (you can't simply forget and type "x.stuff()" without unwrapping the Optional -- the compiler will yell at you). It also separates the concerns of checking-for-validity and doing-things-with-the-instance.

In this case, it looks like we'd make_unique a copy of a SwiftASTContextReader, even if the original was invalid. We no longer do that. For a different kind of example, note how tons of functions have to repeatedly validity-check CompilerType in lldb. The same code smell doesn't exist in llvm, where there is simply no notion of an invalid Type.

[vedantk]

@swift-ci test Linux platform

[adrian-prantl]

I like the argument that everything following an "optional" pattern should literally be an llvm::Optional, that's a good point!
With the Reader being always valid, why does SwiftASTContextForExpressions *get() still return a pointer? Could we hide this indirection by defining an operator SwiftASTContextForExpression &() or some other trick? It's a little weird having to dereference the contents of the Optional even though it is guaranteed to be non-null.

[vedantk]

I agree it's a bit weird. I left 'get' as-is for convenience, since a large number of APIs expect SwiftASTContext *. I'm open to changing that, although maybe we should look at changing those APIs first?

[adrian-prantl]

What do you think about a tiny patch for the swift-5.3 branch that just fixes the programming error, and the idealized refactoring for master/master-next?

[adrian-prantl]

Since all APIs taking a SwiftASTContext * probably return an error if it is null, we might even be able to delete some code by converting it to a reference.

[vedantk]

I'll spin off a more targeted bugfix for 5.3. Thanks!

[adrian-prantl]

If you haven't done so already, can you please also cherry-pick this to swift/master-next?
We don't have an automerger in that direction.

[vedantk]

Just finished the merge.