Don't canonicalize invalid GenericFunctionTypes in diagnostics #38327
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…xt() A TypeBase stores either an ASTContext if its canonical, or its canonical type if it is not. getASTContext() will then canonicalize the type if its not canonical. While this is very clever, in the case of a GenericFunctionType, it triggers the rather expensive creation of a GenericSignatureBuilder. Avoid this by special-casing GenericFunctionType here and getting the ASTContext from a structural sub-component that is known to exist instead.
hborla
approved these changes
Jul 9, 2021
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test macOS |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When emitting diagnostics we sometimes build GenericFunctionTypes with some requirements omitted, for brevity.
Unfortunately a GenericFunctionType with requirements omitted causes difficulties for
getCanonicalType(), which tries to replace interface types inside the function type with their canonical representatives by calling into the GSB. While previously the GSB would accept various invalid inputs, the new requirement machine implementation is stricter and will assert if you pass an invalid interface type.I found three places where types were canonicalized in diagnostics:
TypeBase::getASTContext()computes the canonical type, because a TypeBase stores either an ASTContext (if its canonical) or its canonical type (if its not canonical). Since canonicalizing a GenericFunctionType can be very expensive, this adds a fast path; if the type in question is a GenericFunctionType, we instead call getASTContext() on a structural sub-component that is known to exist (I picked the first generic parameter).shouldShowAKA()was callinggetCanonicalType(), but it should just usegetAkaTypeForDisplay().typeSpellingIsAmbiguous()was callingTypeBase::isEqual()which canonicalizes both sides, but it should just use pointer equality.A better long term solution is to not build invalid GenericFunctionTypes at all; instead, we should use PrintOptions to enable various forms of filtering and customization for the generic signature that is output. But for now, these changes work, and they don't make anything worse, in my opinion.