-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Sema: Request the layout for generic parameters if there is a body #14411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sema: Request the layout for generic parameters if there is a body #14411
Conversation
Generic parameters can become sources of metadata types and conformances. To access them the layout needs to be available to IRGen. rdar://37086144 SR-6879
@swift-ci Please test |
@swift-ci Please test source compatibility |
To be more clear this is requesting the layout for the decl of bound generic types ... |
Build failed |
lib/Sema/TypeCheckPattern.cpp
Outdated
GenericTypeResolver &resolver) { | ||
bool TypeChecker::typeCheckParameterList( | ||
ParameterList *PL, DeclContext *DC, TypeResolutionOptions options, | ||
GenericTypeResolver &resolver, bool bodyCouldRequireTypeOrConformance) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typeCheckParameterList()
can get called for functions that are in non-primary source files (but which could have bodies), so this will require nominal layouts that aren't strictly needed. I recommend that the call to typeCheckAbstractFunctionBody
go into TypeChecker::typeCheckAbstractFunctionBody
, which is only called when we're type-checking the body itself. That more accurately represents when we'll need the layouts.
lib/Sema/TypeCheckPattern.cpp
Outdated
void TypeChecker::requestRequiredNominalTypeLayoutForParameters( | ||
ParameterList *PL) { | ||
for (auto param : *PL) { | ||
auto type = param->hasType() ? param->getType()->getCanonicalType() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll always have a type as this point; did you add the hasType()
because something broke?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically, we wouldn't use getCanonicalType()
here, because we don't actually need it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check for hasType was necessary when called from typeCheckParameterList. This no longer seems necessary after the changes you suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
lib/Sema/TypeCheckPattern.cpp
Outdated
: param->getTypeLoc().getType(); | ||
if (!type) | ||
continue; | ||
if (auto *generic = dyn_cast<BoundGenericType>(type.getPointer())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and then we'd use type->getAs<BoundGenericType>()
here to look through sugar. You can also use type->getAnyGeneric()
to get the generic decl automatically.
@swift-ci Please test |
@swift-ci Please test source compatibility |
Build failed |
Build failed |
…or accessors This seems to be handled by typeCheckAbstractFunctionBody
@swift-ci Please test |
@swift-ci Please test source compatibility |
Build failed |
Build failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fantastic, thanks!
The successful source compat suite run with 19d3b0d is here: https://ci.swift.org/view/Pull%20Request/job/swift-PR-source-compat-suite/649/ |
Cherry-picked: "Merge pull request swiftlang#14411 from aschwaighofer/sema_require_layout_generic_params" Generic parameters can become sources of metadata types and conformances. To access them the layout needs to be available to IRGen. rdar://37086144 SR-6879 * Explanation: With swift-4.1-branch we made requiring the layout of nominal types more lazy. Too lazy, in the case of parameters that are nominal and dependent because such parameters can be used to recover the bound generic type variables (and their conformances) by IRGen which requires their layout. * Scope: Introduced with swift-4.1-branch (see above). Causes runtime crash because type metadata / conformances are loaded from wrong offsets. * Testing: Swift CI tests added. The project from the bug report executes successfully after patch. * Risk: Low. We add a requirement to typecheck the nominal layout (which we did pre swift-4.1 lazyness. * Reviewed: Doug G
Cherry-picked: "Merge pull request swiftlang#14411 from aschwaighofer/sema_require_layout_generic_params" Generic parameters can become sources of metadata types and conformances. To access them the layout needs to be available to IRGen. rdar://37086144 SR-6879
Generic parameters can become sources of metadata types and
conformances. To access them the layout needs to be available to IRGen.
rdar://37086144
SR-6879