SE-0309: Unlock existential types for all protocols #33767

theblixguy · 2020-09-02T19:40:23Z

Implementation for https://forums.swift.org/t/lifting-the-self-or-associated-type-constraint-on-existentials/18025

This PR removes the existing "Self or associated type" restriction. Luckily, the implementation involves only removing some code & we already have the right set of restrictions in place to disallow "invalid" code.

theblixguy · 2020-09-02T19:40:34Z

@swift-ci please test

AnthonyLatsis · 2020-09-02T20:04:50Z

@theblixguy Do you mind if we combine this with swiftlang/swift-evolution#1170 as an evolution proposal? I reached out to Joe yesterday as soon as I realized it would be easier to lift the limitation at once, apart from handling the "known associated type" case.

theblixguy · 2020-09-02T20:10:52Z

@AnthonyLatsis Yeah, sure, I’m open to collaboration!

theblixguy · 2020-09-02T21:06:29Z

Not bad, only 9 tests had to be updated. I have added some additional type checker and execution tests.

theblixguy · 2020-09-02T21:06:38Z

@swift-ci please smoke test

lib/Sema/TypeCheckType.cpp

theblixguy · 2020-09-02T23:19:03Z

I have added various test cases (and updated existing ones), but please let me know if there's anything I have missed.

test/decl/protocol/protocols_with_self_or_assoc_reqs.swift

lib/Sema/TypeCheckType.cpp

owenv · 2020-09-03T03:15:09Z

test/decl/protocol/protocols_with_self_or_assoc_reqs.swift

+
+let p1: P1 = S1()
+_ = p1.returnSelf() // ok
+_ = p1.returnAssoc() // expected-error {{member 'returnAssoc' cannot be used on value of protocol type 'P1'; use a generic constraint instead}}


It would be really great if we could change this to something more detailed like member 'returnAssoc' cannot be used on value of protocol type 'P1'; because it returns associated type 'Q', it may only be used on a concrete conforming type, since if the proposal is accepted users will encounter it much more often

On that note, it might be better to update the related educational note to explain why these members can't be used, rather than remove the educational note altogether.

Turns out there's already at least one use of Self that's allowed, which is difficult to explain. I've left #33736 as a draft so that the educational note can evolve in parallel with these changes.

lib/Sema/CSApply.cpp

lib/AST/Decl.cpp

lib/AST/ASTContext.cpp

shahmishal · 2020-10-01T06:15:27Z

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

How to change the base branch: (Link)
More detail about the branch update: (Link)

This will enable us to allow members containing references to Self or associated types only in covariant position to be used on an existential

AST: Refactor SelfReferenceKind in preparation for #33767

Mordil · 2021-04-14T18:32:30Z

Could we get a toolchain built for this so we can play with it?

(I wasn't sure if anyone can trigger the CI to build one or not)

AnthonyLatsis · 2021-04-14T19:44:11Z

@Mordil Sure, we will build a toolchain as soon as we push our downstreams.

Mordil · 2021-04-14T20:29:50Z

@AnthonyLatsis ❤️ thank you!

ctxppc · 2021-04-16T21:03:49Z

lib/AST/ASTContext.cpp

+  }
+
+  // Bound generic types are invariant.
+  if (auto boundGenericType = type->getAs<BoundGenericType>()) {


I’m trying to figure out how it all fits together (out of sheer curiosity 😛) but I can’t seem to find the part that implements the special-case covariant Optional.Wrapped, Array.Element, and Dictionary.Value type parameters. As far as I can understand, this function classifies these as invariant (just like any other bound generic type). Is the special-case covariance in this (or a future) PR? Thank you, and thank you too for the helpful inline documentation!

Special-case covariance is already implemented on the main branch. This branch needs a rebase.

… educational note

…rtedProtocolType()

…colSelfReferences»

…xistential member access

AnthonyLatsis · 2021-08-16T15:43:14Z

@swift-ci please smoke test

AnthonyLatsis · 2021-08-16T17:13:55Z

@swift-ci please smoke test macOS

AnthonyLatsis · 2021-08-16T19:38:19Z

@swift-ci please test Windows

ktoso · 2021-08-19T03:28:26Z

@swift-ci please test Windows Platform

AnthonyLatsis · 2021-08-19T18:43:42Z

@swift-ci please test Windows

Saklad5 · 2021-08-23T21:08:25Z

Is this on-track to make it into Swift 5.5?

ktoso · 2021-08-26T20:59:31Z

Checked in with @jckarter who was review manager for the evolution proposal -- let's go ahead and merge this 🎉

Awesome work @AnthonyLatsis, thanks!

…ve-self-or-associated-type-diagnostic" The following regression test added for this feature is not passing: Swift(linux-x86_64) :: decl/protocol/protocols_with_self_or_assoc_reqs_executable.swift with a compiler crash happening during SILFunctionTransform "Devirtualizer". Reverting to unblock CI. This reverts commit f96057e, reversing changes made to 3fc18f3.

Revert "Merge pull request #33767 ..."

kavon · 2021-08-26T23:57:29Z

I reverted this PR only because one of the tests added for this feature was failing right after being merged. See PR #39067 for details. Something must have changed on main the 10 days since the tests passed, or maybe the smoke test didn't catch it (try a full test?). Please try merging it again after fixing that test 😃 . I'm not sure if that requires making a new PR or not?

ktoso · 2021-08-27T00:05:18Z

Oh no! Thanks for catching it @kavon !

I'm not sure if that requires making a new PR or not?

New PR is likely best.

AnthonyLatsis · 2021-08-27T00:19:18Z

Drat! Looking into this...

AnthonyLatsis · 2021-08-30T18:17:58Z

Not good... The crash is unrelated to this change, but it happens this bug was still introduced by me in #34140 (sorry!). The problem is that the SIL optimizer is failing to devirtualize a call to a protocol requirement like foo() -> [Self], with covariant Self nested inside a known-covariant stdlib type such as an array or dictionary, because they involve a consecutive intrinsic collection cast apply that needs to be found and replaced too, or a thunk partial_apply when stuff like [Self] appears in other covariant positions. I've tried looking into fixing this, but it's my first time digging into the optimizer, and I really need a bit of guidance. For now, I've made the test work with @_optimize(none) — #39095. @jckarter At this conjecture, should we be reverting #34140 first?

cc: @ktoso

ktoso · 2021-09-02T02:17:57Z

Sounds like reverting #34140 and getting you some help/guidance to get it right would be good. Joe is vacationing now but will be back soon, maybe @slavapestov can offer some guidance?

And we could merge #39095 in the meantime 👍

slavapestov · 2021-09-02T03:40:48Z

@AnthonyLatsis in the mean time can we tweak the optimizer to not attempt to devirtualize such calls? Then you can land the real fix later at your leisure, without blocking anything else.

AnthonyLatsis · 2021-09-02T04:17:14Z

@slavapestov Sure, thanks for the go-ahead. I initially thought the code there was too performance-sensitive for a defensive type traversal.

ZsoltMolnarMBH · 2021-10-15T18:20:52Z

Dear contributors, (@theblixguy)

I cannot seem to figure out wether this change is still in the main branch or not.
What is the status of this feature?
When should we expect to put this to use?

AnthonyLatsis · 2021-10-15T19:03:46Z

This specific change is merged, and will be included in the next Swift release, whenever that happens. There is a series of changes yet to come to fully implement the proposal, though (see this thread for details).

theblixguy changed the title ~~[DNM] [WIP] Lift the 'Self or associated type' restriction~~ [DNM] Lift the 'Self or associated type' restriction Sep 2, 2020

swiftlang deleted a comment from swift-ci Sep 2, 2020

slavapestov reviewed Sep 2, 2020

View reviewed changes

lib/Sema/TypeCheckType.cpp Show resolved Hide resolved

theblixguy added the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Sep 2, 2020

AnthonyLatsis reviewed Sep 2, 2020

View reviewed changes

test/decl/protocol/protocols_with_self_or_assoc_reqs.swift Outdated Show resolved Hide resolved

slavapestov reviewed Sep 3, 2020

View reviewed changes

lib/Sema/TypeCheckType.cpp Show resolved Hide resolved

owenv reviewed Sep 3, 2020

View reviewed changes

AnthonyLatsis mentioned this pull request Sep 4, 2020

Unlock Existential Types for All Protocols swiftlang/swift-evolution#1176

Merged

AnthonyLatsis reviewed Sep 5, 2020

View reviewed changes

lib/Sema/CSApply.cpp Outdated Show resolved Hide resolved

xwu reviewed Sep 5, 2020

View reviewed changes

lib/AST/Decl.cpp Outdated Show resolved Hide resolved

AnthonyLatsis reviewed Sep 8, 2020

View reviewed changes

lib/AST/ASTContext.cpp Outdated Show resolved Hide resolved

AnthonyLatsis changed the base branch from master to main October 1, 2020 06:24

AnthonyLatsis added a commit that referenced this pull request Oct 13, 2020

Merge pull request #34249 from AnthonyLatsis/selfrefkind-refactor

ec10c5d

AST: Refactor SelfReferenceKind in preparation for #33767

ctxppc reviewed Apr 16, 2021

View reviewed changes

AnthonyLatsis changed the title ~~[DNM] Lift the 'Self or associated type' restriction~~ SE-0309: Unlock existential types for all protocols Apr 25, 2021

jckarter added the swift evolution approved Flag → feature: A feature that was approved through the Swift evolution process label May 4, 2021

AnthonyLatsis and others added 8 commits August 16, 2021 18:30

Docs: Reattach, rewrite and rename the «associated-type-requirements»…

fbae8a4

… educational note

[Test] Add additional tests for new behavior

0daf682

[Test] Update existing tests

7834120

Test: Find better locations for some updated tests

749a0ae

[NFC, Typechecker] Remove UnsupportedProtocolVisitor and checkUnsuppo…

23c1d66

…rtedProtocolType()

NFC: Remove the now dead ProtocolDecl::existentialTypeSupported()

755f0f9

AST: Account for protocol compositions and nested types in «findProto…

655a81c

…colSelfReferences»

Sema: Check for invalid reference *before* checking for unsupported e…

9a52220

…xistential member access

ktoso merged commit f96057e into swiftlang:main Aug 26, 2021

kavon mentioned this pull request Aug 26, 2021

Revert "Merge pull request #33767 ..." #39067

Merged

kavon added a commit that referenced this pull request Aug 26, 2021

Merge pull request #39067 from kavon/revert-unlock-existential

bc8657b

Revert "Merge pull request #33767 ..."

theblixguy deleted the chore/remove-self-or-associated-type-diagnostic branch September 10, 2021 12:13

AnthonyLatsis mentioned this pull request Mar 23, 2021

[SR-4758] SE-0142: derived protocol that restricts associatedtype should be allowed as a variable type #47335

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SE-0309: Unlock existential types for all protocols #33767

SE-0309: Unlock existential types for all protocols #33767

theblixguy commented Sep 2, 2020 •

edited

Loading

theblixguy commented Sep 2, 2020

AnthonyLatsis commented Sep 2, 2020 •

edited

Loading

theblixguy commented Sep 2, 2020

theblixguy commented Sep 2, 2020

theblixguy commented Sep 2, 2020

theblixguy commented Sep 2, 2020 •

edited

Loading

owenv Sep 3, 2020

Jumhyn Sep 3, 2020

xwu Sep 3, 2020

shahmishal commented Oct 1, 2020

Mordil commented Apr 14, 2021

AnthonyLatsis commented Apr 14, 2021

Mordil commented Apr 14, 2021 •

edited

Loading

ctxppc Apr 16, 2021

AnthonyLatsis Apr 20, 2021

AnthonyLatsis commented Aug 16, 2021

AnthonyLatsis commented Aug 16, 2021

AnthonyLatsis commented Aug 16, 2021

ktoso commented Aug 19, 2021

AnthonyLatsis commented Aug 19, 2021

Saklad5 commented Aug 23, 2021

ktoso commented Aug 26, 2021 •

edited

Loading

kavon commented Aug 26, 2021 •

edited

Loading

ktoso commented Aug 27, 2021 •

edited

Loading

AnthonyLatsis commented Aug 27, 2021

AnthonyLatsis commented Aug 30, 2021 •

edited

Loading

ktoso commented Sep 2, 2021

slavapestov commented Sep 2, 2021

AnthonyLatsis commented Sep 2, 2021

ZsoltMolnarMBH commented Oct 15, 2021

AnthonyLatsis commented Oct 15, 2021 •

edited

Loading

SE-0309: Unlock existential types for all protocols #33767

SE-0309: Unlock existential types for all protocols #33767

Conversation

theblixguy commented Sep 2, 2020 • edited Loading

theblixguy commented Sep 2, 2020

AnthonyLatsis commented Sep 2, 2020 • edited Loading

theblixguy commented Sep 2, 2020

theblixguy commented Sep 2, 2020

theblixguy commented Sep 2, 2020

theblixguy commented Sep 2, 2020 • edited Loading

owenv Sep 3, 2020

Choose a reason for hiding this comment

Jumhyn Sep 3, 2020

Choose a reason for hiding this comment

xwu Sep 3, 2020

Choose a reason for hiding this comment

shahmishal commented Oct 1, 2020

Mordil commented Apr 14, 2021

AnthonyLatsis commented Apr 14, 2021

Mordil commented Apr 14, 2021 • edited Loading

ctxppc Apr 16, 2021

Choose a reason for hiding this comment

AnthonyLatsis Apr 20, 2021

Choose a reason for hiding this comment

AnthonyLatsis commented Aug 16, 2021

AnthonyLatsis commented Aug 16, 2021

AnthonyLatsis commented Aug 16, 2021

ktoso commented Aug 19, 2021

AnthonyLatsis commented Aug 19, 2021

Saklad5 commented Aug 23, 2021

ktoso commented Aug 26, 2021 • edited Loading

kavon commented Aug 26, 2021 • edited Loading

ktoso commented Aug 27, 2021 • edited Loading

AnthonyLatsis commented Aug 27, 2021

AnthonyLatsis commented Aug 30, 2021 • edited Loading

ktoso commented Sep 2, 2021

slavapestov commented Sep 2, 2021

AnthonyLatsis commented Sep 2, 2021

ZsoltMolnarMBH commented Oct 15, 2021

AnthonyLatsis commented Oct 15, 2021 • edited Loading

theblixguy commented Sep 2, 2020 •

edited

Loading

AnthonyLatsis commented Sep 2, 2020 •

edited

Loading

theblixguy commented Sep 2, 2020 •

edited

Loading

Mordil commented Apr 14, 2021 •

edited

Loading

ktoso commented Aug 26, 2021 •

edited

Loading

kavon commented Aug 26, 2021 •

edited

Loading

ktoso commented Aug 27, 2021 •

edited

Loading

AnthonyLatsis commented Aug 30, 2021 •

edited

Loading

AnthonyLatsis commented Oct 15, 2021 •

edited

Loading