Skip to content

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

Conversation

aschwaighofer
Copy link
Contributor

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
@aschwaighofer
Copy link
Contributor Author

@swift-ci Please test

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please test source compatibility

@aschwaighofer
Copy link
Contributor Author

To be more clear this is requesting the layout for the decl of bound generic types ...

@swift-ci
Copy link
Contributor

swift-ci commented Feb 5, 2018

Build failed
Swift Test Linux Platform
Git Sha - ea63089

GenericTypeResolver &resolver) {
bool TypeChecker::typeCheckParameterList(
ParameterList *PL, DeclContext *DC, TypeResolutionOptions options,
GenericTypeResolver &resolver, bool bodyCouldRequireTypeOrConformance) {
Copy link
Member

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.

void TypeChecker::requestRequiredNominalTypeLayoutForParameters(
ParameterList *PL) {
for (auto param : *PL) {
auto type = param->hasType() ? param->getType()->getCanonicalType()
Copy link
Member

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?

Copy link
Member

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...

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

: param->getTypeLoc().getType();
if (!type)
continue;
if (auto *generic = dyn_cast<BoundGenericType>(type.getPointer())) {
Copy link
Member

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.

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please test

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please test source compatibility

@swift-ci
Copy link
Contributor

swift-ci commented Feb 6, 2018

Build failed
Swift Test OS X Platform
Git Sha - ea63089

@swift-ci
Copy link
Contributor

swift-ci commented Feb 6, 2018

Build failed
Swift Test Linux Platform
Git Sha - ea63089

…or accessors

This seems to be handled by typeCheckAbstractFunctionBody
@aschwaighofer
Copy link
Contributor Author

@swift-ci Please test

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please test source compatibility

@swift-ci
Copy link
Contributor

swift-ci commented Feb 6, 2018

Build failed
Swift Test OS X Platform
Git Sha - 2f98bb1

@swift-ci
Copy link
Contributor

swift-ci commented Feb 6, 2018

Build failed
Swift Test Linux Platform
Git Sha - 2f98bb1

Copy link
Member

@DougGregor DougGregor left a 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!

@aschwaighofer
Copy link
Contributor Author

The successful source compat suite run with 19d3b0d is here:

https://ci.swift.org/view/Pull%20Request/job/swift-PR-source-compat-suite/649/

@aschwaighofer aschwaighofer merged commit 5f595ac into swiftlang:master Feb 6, 2018
aschwaighofer added a commit to aschwaighofer/swift that referenced this pull request Feb 6, 2018
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
aschwaighofer added a commit to aschwaighofer/swift that referenced this pull request Feb 6, 2018
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants