Skip to content

[SE-0042][AST/Sema/SILGen] Flattening the function type of unapplied method references #3836

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

Closed
wants to merge 9 commits into from

Conversation

JaSpa
Copy link
Contributor

@JaSpa JaSpa commented Jul 28, 2016

What's in this pull request?

This implements the proposal SE-0042.
With it come a lot of test changes and sadly some regressions in quality of the diagnostics.

Implemented proposal: SE-0042 (SR-1051)

cc @tkremenek


Before merging this pull request to apple/swift repository:

  • Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform Comment
All supported platforms @swift-ci Please smoke test
All supported platforms @swift-ci Please smoke test and merge
OS X platform @swift-ci Please smoke test OS X platform
Linux platform @swift-ci Please smoke test Linux platform

A smoke test on macOS does the following:

  1. Builds the compiler incrementally.
  2. Builds the standard library only for macOS. Simulator standard libraries and
    device standard libraries are not built.
  3. lldb is not built.
  4. The test and validation-test targets are run only for macOS. The optimized
    version of these tests are not run.

A smoke test on Linux does the following:

  1. Builds the compiler incrementally.
  2. Builds the standard library incrementally.
  3. lldb is built incrementally.
  4. The swift test and validation-test targets are run. The optimized version of these
    tests are not run.
  5. lldb is tested.

Validation Testing

Platform Comment
All supported platforms @swift-ci Please test
All supported platforms @swift-ci Please test and merge
OS X platform @swift-ci Please test OS X platform
OS X platform @swift-ci Please benchmark
Linux platform @swift-ci Please test Linux platform

Lint Testing

Language Comment
Python @swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

This patch basically just simplifies the constructors, where in 6 out of 8 cases `isSpecialized = false`
@tkremenek
Copy link
Member

@swift-ci smoke test

@tkremenek tkremenek self-assigned this Jul 28, 2016

/// Set whether this function conversion flattens an unapplied member
/// function.
void setFlattening() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better to make this its own type of Expr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought about this as well, but there's exactly one place where we set this flag …

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, so that place can make the new Expr type instead. These really are not function conversions...

@slavapestov
Copy link
Contributor

This is a good start, but I have some issues with the approach. I'd like to see this integrated better with the existing curry thunk stuff. Instead of emitting a curried SILDeclRef and wrapping it in a thunk, it makes more sense to directly emit it at the right uncurry level.

@slavapestov slavapestov assigned slavapestov and unassigned tkremenek Jul 29, 2016
@@ -360,6 +360,14 @@ class alignas(8) Expr {
< (1 << NumCheckedCastKindBits),
"unable to fit a CheckedCastKind in the given number of bits");

class FunctionConversionExprBitfields {
friend class FunctionConversionExpr;
unsigned : NumExprBits;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use NumImplicitConversionBits here and below.

JaSpa added 7 commits July 29, 2016 03:21
Reabstraction thunks are now used for this flattening/uncurrying as well.
First the self parameter is applied, the variables are updated not to include
this first parameter and then the "normal" reabstraction takes place.
Some diagnostics got very bad due to the changes...
->getUncurriedFunction();
result = coerceToType(
result, newTy, locator.withPathElement(ConstraintLocator::Member));
cast<FunctionConversionExpr>(result)->setFlattening();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the full coerceToType treatment necessary? You're assuming that it will go down one specific path and produce one specific result anyway; just make the FunctionConversionExpr here.

@JaSpa
Copy link
Contributor Author

JaSpa commented Jul 29, 2016

Thank you for your time reviewing this, @slavapestov @rjmccall!
I tried to hack a small working proof of concept together with the new approach mentioned by Slava, but I just don't have the expertise to get it working in the next hours. But if needed/wanted I'd be willing to continue working on this (but this would've to wait until after my vacation).
I'll address the remaining review comments and then put the fate of this patch into your hands. That it felt like the wrong path (reabstraction thunk here and there) during coding can't be denied, but it was to me the easiest way to get it working. To me it would be great to see it in the release despite “the not so good” implementation 😅, but still Doug's other changes already caused quite a few conflicts so maybe to wait until this has landed isn't the worst way.

@slavapestov
Copy link
Contributor

Can we close this? We will probably go with a different approach for this feature -- we want to retool thunks to have the new uncurried type by default, adding the swift 3 compatibility behavior on top.

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.

4 participants