Skip to content

Conversation

@huonw
Copy link
Contributor

@huonw huonw commented Jan 25, 2017

This is a generic signature that stores exactly the requirements that a
protocol decl introduces, not letting them be implied by the Self :
Protocol requirement, nor storing any requirements introduced by the
protocols requirements.

Specifically, suppose we have

protocol Foo {}
protocol Bar {}

protocol Baz {
    associatedtype X : Foo
}
protocol Qux: Baz {
    associatedtype X : Bar
}

The normal generic signature and (canonical) protocol requirement
signature of Baz will be, respectively

<Self where Self : Baz>
<Self where Self.X : Foo>

And for Qux, they will be:

<Self where Self : Qux>
<Self where Self : Baz, Self.X : Bar>

Note that the Self.X : Foo requirement is not listed.

For the moment, this is unused except for -debug-generic-signatures.

@huonw huonw requested a review from DougGregor January 25, 2017 18:49
@huonw huonw force-pushed the protocol-self-signature branch 2 times, most recently from b07e0c1 to 28d2234 Compare January 25, 2017 19:43
@huonw
Copy link
Contributor Author

huonw commented Jan 25, 2017

@swift-ci Please test.

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.

The functionality looks great, but I'd prefer a simpler API on ProtocolDecl.

Copy link
Member

Choose a reason for hiding this comment

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

I know this is just a refactoring of what was there before, but it seems odd to use RequirementSource::Redundant here rather than RequirementSource::Protocol. Maybe we should (separately!) see if we can make that change to align it with what we do below.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the right API. addGenericSignature() is meant to introduce an arbitrary signature, but when protocolRequirementSig == true, the constraints on that signature are so specific that they'd be better served by calling them out via a new API. Moreover, a client of ArchetypeBuilder can do all of this work itself. For a given protocol proto, get the protocol's Self interface type as a GenericTypeParamType * and call addGenericParameter(), then addRequirement with a newly-synthesized conformance Requirement of Self: proto and your newly-created ProtocolRequirementSignatureSelf source.

To be really specific, I think this API should be on ProtocolDecl:

GenericSignature *getRequirementSignature();

You can make it perform on-demand computation for now (creating a new ArchetypeBuilder and following the above process) while we're just dumping() the results for debugging, then start caching the result on the AST, serializing it, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds like it will be much better.

Copy link
Member

Choose a reason for hiding this comment

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

This is the bit that convinced me that we don't want the protocolRequirementSig parameter.

Copy link
Member

Choose a reason for hiding this comment

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

The "Self: P1" conformance shouldn't be there in the resulting signature, should 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.

Why not? Do you mean we should treat ProtocolRequirementSignatureSelf like they're redundant? I recall from earlier discussions that we'd just drop this requirement when doing later processing, but it would also make sense to just not include it in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

We can ask clients to ignore it, I guess. It just feels wrong to have this requirement in the signature because it makes all of the other requirements redundant.

@swiftix
Copy link
Contributor

swiftix commented Jan 25, 2017

@huonw What is the intended use for this feature?

@DougGregor
Copy link
Member

@swiftix it provides the set of new requirements introduced by a protocol in a semantic form, minimized and canonicalized. There are a bunch of places we want to use it:

  • In the archetype builder, so we don't need to re-walk the contents of a ProtocolDecl to expand out its requirement
  • In the constraint solver, for the same reason
  • In IRGen for witness tables, so we know how to store the witness tables of additional protocol conformances that this protocol introduces

@swiftix
Copy link
Contributor

swiftix commented Jan 25, 2017

@huonw Thanks for the explanation!

@huonw huonw force-pushed the protocol-self-signature branch 2 times, most recently from e05210e to aa6b6a1 Compare January 25, 2017 21:38
Copy link
Member

Choose a reason for hiding this comment

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

Please put a doc comment here so it's clear to the reader what the requirement signature actually is. You can basically use the text of your commit message here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, whoops. Forgot to circle back and do that.

@huonw huonw force-pushed the protocol-self-signature branch from aa6b6a1 to a180df0 Compare January 25, 2017 22:01
This is a generic signature that stores exactly the requirements that a
protocol decl introduces, not letting them be implied by the Self :
Protocol requirement, nor storing any requirements introduced by the
protocols requirements.

Specifically, suppose we have

    protocol Foo {}
    protocol Bar {}

    protocol Baz {
        associatedtype X : Foo
    }
    protocol Qux: Baz {
        associatedtype X : Bar
    }

The normal generic signature and (canonical) protocol requirement
signature of `Baz` will be, respectively

    <Self where Self : Baz>
    <Self where Self : Baz, Self.X : Foo>

And for `Qux`, they will be:

    <Self where Self : Qux>
    <Self where Self : Qux, Self : Baz, Self.X : Bar>

Note that the `Self.X : Foo` requirement is not listed.

For the moment, this is unused except for `-debug-generic-signatures`.
@huonw huonw force-pushed the protocol-self-signature branch from a180df0 to a964b6a Compare January 26, 2017 00:07
@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 28d22342594d15cc4b15970b304771d0ffee670c
Test requested by - @huonw

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 28d22342594d15cc4b15970b304771d0ffee670c
Test requested by - @huonw

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 version looks fantastic, thanks!

@huonw
Copy link
Contributor Author

huonw commented Jan 26, 2017

@swift-ci Please test and merge.

@DougGregor
Copy link
Member

Looks like CI didn't get through the macOS build; the other smoke tests suffice, so I'm going to merge this now.

@DougGregor DougGregor merged commit 9b99d26 into swiftlang:master Jan 26, 2017
@huonw huonw deleted the protocol-self-signature branch January 26, 2017 18:22
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