AST, Sema: Teach findProtocolSelfReferences that some stdlib collections preserve variance #34140

AnthonyLatsis · 2020-10-01T02:02:16Z

Extending this to other covariant stdlib collections is problematic due to the Hashable requirement.

test/decl/func/dynamic_self.swift

slavapestov

~~I think this is big enough of a change to warrant an evolution proposal.~~ I had a brain fart and thought this was about covariance in witness matching and not calling on an existential. Still, we should get a second opinion about the soundness of this.

Does SILGen actually generate the correct conversion from Array<Self> to Array<P> when you call an Array<Self>-returning method on an existential?

AnthonyLatsis · 2020-10-01T06:20:06Z

Ought the conversion to be special compared to other Array upcasts? The SIL generated for let x = p.foo() looks pretty run-of-the-mill:

// takesP(arg:)
sil hidden [ossa] @$s4test6takesP3argyAA1P_p_tF : $@convention(thin) (@in_guaranteed P) -> () {
// %0 "arg"
bb0(%0 : $*P):
  debug_value_addr %0 : $*P, let, name "arg", argno 1
  %2 = open_existential_addr immutable_access %0 : $*P to $*@opened(X) P
  %3 = witness_method $@opened(X) P, #P.foo {{.*}} : $@convention(witness_method: P) <τ_0_0 where τ_0_0 : P> (@in_guaranteed τ_0_0) -> @owned Array<τ_0_0>
  %4 = apply %3<@opened(X) P>(%2) : $@convention(witness_method: P) <τ_0_0 where τ_0_0 : P> (@in_guaranteed τ_0_0) -> @owned Array<τ_0_0>
  // function_ref _arrayForceCast<A, B>(_:)
  %5 = function_ref @$ss15_arrayForceCastySayq_GSayxGr0_lF : $@convention(thin) <τ_0_0, τ_0_1> (@guaranteed Array<τ_0_0>) -> @owned Array<τ_0_1>
  %6 = apply %5<@opened(X) P, P>(%4) : $@convention(thin) <τ_0_0, τ_0_1> (@guaranteed Array<τ_0_0>) -> @owned Array<τ_0_1>
  debug_value %6 : $Array<P>, let, name "x"
  destroy_value %4 : $Array<@opened(X) P>
  destroy_value %6 : $Array<P>
  %10 = tuple ()
  return %10 : $()
}

// _arrayForceCast<A, B>(_:)
sil [serialized] @$ss15_arrayForceCastySayq_GSayxGr0_lF : $@convention(thin) <τ_0_0, τ_0_1> (@guaranteed Array<τ_0_0>) -> @owned Array<τ_0_1>

I am always happy to add more tests though!

AnthonyLatsis · 2020-12-02T13:46:41Z

@jckarter ping

jckarter · 2020-12-02T18:27:46Z

@swift-ci Please test

jckarter · 2020-12-02T18:27:56Z

@swift-ci Please test source compatibility

swift-ci · 2020-12-02T19:30:04Z

Build failed
Swift Test Linux Platform
Git Sha - 87a5d058544c23163561aca14cb438825b8c6efe

swift-ci · 2020-12-02T19:42:21Z

Build failed
Swift Test OS X Platform
Git Sha - 87a5d058544c23163561aca14cb438825b8c6efe

AnthonyLatsis · 2020-12-02T19:57:40Z

There was a conflict.

@swift-ci please test

AnthonyLatsis · 2020-12-02T19:58:05Z

@swift-ci please test source compatibility

jckarter · 2020-12-02T20:18:30Z

I think this looks good, as long as there aren't any source compat subtleties.

swift-ci · 2020-12-02T21:59:22Z

Build failed
Swift Test Linux Platform
Git Sha - c691b4f9c5dd0be968eb502c0ae9f344a395db2a

AnthonyLatsis · 2020-12-03T04:40:06Z

@jckarter Thanks for taking the time to have a look!

@slavapestov Do you have any additional comments?

@swift-ci please test Linux

slavapestov

Looks good. One more thing -- do you mind adding an entry to CHANGELOG.md? You can do that in a separate PR if you like.

…ons preserve variance * Swift.Array preserves variance in its 'Element' type * Swift.Dictionary preserves variance in its 'Value' type

AnthonyLatsis · 2021-02-11T01:25:39Z

@swift-ci please test

AnthonyLatsis · 2021-02-11T01:25:53Z

@swift-ci please test source compatibility

AnthonyLatsis · 2021-02-11T15:02:02Z

@swift-ci please smoke test

AnthonyLatsis · 2021-02-12T13:46:05Z

@swift-ci please test source compatibility DEBUG

xwu · 2021-02-13T21:11:30Z

include/swift/AST/DiagnosticsSema.def

@@ -2931,8 +2931,8 @@ ERROR(dynamic_self_non_method,none,
      "%select{global|local}0 function cannot return 'Self'", (bool))

 ERROR(dynamic_self_invalid,none,
-      "covariant 'Self' can only appear as the type of a property, subscript or method result; "
-      "did you mean '%0'?", (StringRef))
+      "covariant 'Self' can only appear as the possibly optional type of a "


Does this sentence mean simply covariant Self or Self?? I struggle a little with how to parse “possibly optional type.”

The intention was to convey that both Self and Self? are supported. Do you happen to have a suggestion for a better wording?

Perhaps simply: "covariant 'Self' or 'Self?' can only appear as..."

Heh, indeed. Thank you, I'll take care of this!

Add CHANGELOG entry for #34140

… collection The devirtualizer does not support handling these yet, which wasn't anticipated in swiftlang#34140

AnthonyLatsis requested review from xedin and slavapestov October 1, 2020 02:02

AnthonyLatsis changed the title ~~Coself array~~ AST, Sema: Teach findProtocolSelfReferences that Swift.Array preserves variance of its Element Oct 1, 2020

LucianoPAlmeida reviewed Oct 1, 2020

View reviewed changes

test/decl/func/dynamic_self.swift Show resolved Hide resolved

slavapestov reviewed Oct 1, 2020

View reviewed changes

AnthonyLatsis force-pushed the coself-array branch from 2578323 to be016c8 Compare October 1, 2020 19:02

AnthonyLatsis changed the title ~~AST, Sema: Teach findProtocolSelfReferences that Swift.Array preserves variance of its Element~~ AST, Sema: Teach findProtocolSelfReferences that some stdlib collections preserve variance Oct 1, 2020

AnthonyLatsis requested review from DougGregor and jckarter October 2, 2020 21:06

AnthonyLatsis force-pushed the coself-array branch from 87a5d05 to c691b4f Compare December 2, 2020 19:55

slavapestov approved these changes Dec 15, 2020

View reviewed changes

AnthonyLatsis force-pushed the coself-array branch from c691b4f to 3ca7cc6 Compare February 11, 2021 00:07

AnthonyLatsis added 3 commits February 11, 2021 03:22

AST, Sema: Teach findProtocolSelfReferences that some stdlib collecti…

3d31793

…ons preserve variance * Swift.Array preserves variance in its 'Element' type * Swift.Dictionary preserves variance in its 'Value' type

Diag: Fix inaccuracy in dynamic_self_invalid

06ceb19

Add a SILGen test for collection upcasts

1ee4d5b

AnthonyLatsis force-pushed the coself-array branch from 3ca7cc6 to 1ee4d5b Compare February 11, 2021 00:23

AnthonyLatsis added a commit to AnthonyLatsis/swift that referenced this pull request Feb 13, 2021

Add CHANGELOG entry for swiftlang#34140

d1e2c0c

AnthonyLatsis merged commit a223d37 into swiftlang:main Feb 13, 2021

AnthonyLatsis deleted the coself-array branch February 13, 2021 16:45

xwu reviewed Feb 13, 2021

View reviewed changes

AnthonyLatsis added a commit that referenced this pull request Feb 16, 2021

Merge pull request #35969 from AnthonyLatsis/changelog-coself

d2bada8

Add CHANGELOG entry for #34140

AnthonyLatsis mentioned this pull request Mar 12, 2021

Diag: Reword some errors for invalid usage of covariant Self #36418

Merged

AnthonyLatsis mentioned this pull request Aug 30, 2021

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

Merged

AnthonyLatsis mentioned this pull request Sep 2, 2021

Devirtualize: Skip requirements with covariant 'Self' nested inside a collection #39137

Merged

AnthonyLatsis added a commit to AnthonyLatsis/swift that referenced this pull request Sep 3, 2021

Devirtualize: Skip requirements with covariant 'Self' nested inside a…

b81db7a

… collection The devirtualizer does not support handling these yet, which wasn't anticipated in swiftlang#34140

AST, Sema: Teach findProtocolSelfReferences that some stdlib collections preserve variance #34140

AST, Sema: Teach findProtocolSelfReferences that some stdlib collections preserve variance #34140

Uh oh!

Conversation

AnthonyLatsis commented Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

slavapestov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnthonyLatsis commented Oct 1, 2020

Uh oh!

AnthonyLatsis commented Dec 2, 2020

Uh oh!

jckarter commented Dec 2, 2020

Uh oh!

jckarter commented Dec 2, 2020

Uh oh!

swift-ci commented Dec 2, 2020

Uh oh!

swift-ci commented Dec 2, 2020

Uh oh!

AnthonyLatsis commented Dec 2, 2020

Uh oh!

AnthonyLatsis commented Dec 2, 2020

Uh oh!

jckarter commented Dec 2, 2020

Uh oh!

swift-ci commented Dec 2, 2020

Uh oh!

AnthonyLatsis commented Dec 3, 2020

Uh oh!

slavapestov left a comment

Choose a reason for hiding this comment

Uh oh!

AnthonyLatsis commented Feb 11, 2021

Uh oh!

AnthonyLatsis commented Feb 11, 2021

Uh oh!

AnthonyLatsis commented Feb 11, 2021

Uh oh!

AnthonyLatsis commented Feb 12, 2021

Uh oh!

xwu Feb 13, 2021

Choose a reason for hiding this comment

Uh oh!

AnthonyLatsis Feb 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xwu Feb 14, 2021

Choose a reason for hiding this comment

Uh oh!

AnthonyLatsis Feb 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AnthonyLatsis commented Oct 1, 2020 •

edited

Loading

slavapestov left a comment •

edited

Loading

AnthonyLatsis Feb 13, 2021 •

edited

Loading