Make `GenericSignature::getCanonicalSignature` not crash for nullptr `GenericSignature`?

[dan-zheng]

Currently, GenericSignatureImpl::getCanonicalSignature() crashes for GenericSignature with underlying nullptr. This leads to lots of boilerplate code:

// Ideal:
auto canGenSig = genSig->getCanonicalSignature();

// Workaround:
CanGenericSignature canGenSig;
if (genSig)
  canGenSig = genSig->getCanonicalSignature();

// Workaround:
auto canGenSig =
  genSig ? genSig->getCanonicalSignature() : CanGenericSignature();

Is this intentional? Is it possible to change GenericSignatureImpl::getCanonicalSignature() to return CanGenericSignature() when this is nullptr?

I wonder if TypeBase::getCanonicalType has the same issue (crashing when this is nullptr), and whether that can be changed too.

cc @codafi

[Joe_Groff]

It is undefined behavior to call a method on a null pointer in C++. I would guess that the intent was eventually to make getCanonicalSignature a method directly on the GenericSignature wrapper type, which could check for null before invoking the method on the underlying impl. Eventually, most of the important API for GenericSignature should probably live on GenericSignature, and provide reasonable default behavior for empty generic signatures, so that GenericSignatureImpl can be hidden.

[dan-zheng]

@Joe_Groff

It is undefined behavior to call a method on a null pointer in C++.

Ah, oops.

@Joe_Groff

I would guess that the intent was eventually to make getCanonicalSignature a method directly on the GenericSignature wrapper type, which could check for null before invoking the method on the underlying impl.

That sounds great! I can start a patch to do this and to rewrite caller workarounds, if that's acceptable.

[Joe_Groff]

Sounds great, thanks!

[dan-zheng]

@dan-zheng

That sounds great! I can start a patch to do this and to rewrite caller workarounds, if that's acceptable.

Done in NFC: Add `GenericSignature::getCanonicalSignature`. by dan-zheng · Pull Request #29105 · apple/swift · GitHub, awaiting review.