Skip to content

TypeCheckType: Fix existential any migration diagnostics for extra-modular protocols #65112

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 13, 2023
Merged

TypeCheckType: Fix existential any migration diagnostics for extra-modular protocols #65112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions include/swift/AST/Decl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4889,8 +4889,14 @@ class ProtocolDecl final : public NominalTypeDecl {
bool requiresSelfConformanceWitnessTable() const;

/// Determine whether an existential type must be explicitly prefixed
/// with \c any. \c any is required if any of the members contain
/// an associated type, or if \c Self appears in non-covariant position.
/// with \c any. \c any is required if one of the following conditions is met
/// for this protocol or an inherited protocol:
/// - The protocol has an associated type requirement.
/// - `Self` appears in non-covariant position in the type signature of a
/// value requirement.
///
/// @Note This method does not take the state of language features into
/// account.
Copy link
Member

Choose a reason for hiding this comment

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

You could also pass in the usage context into existentialRequiresAny so the method itself accounts for language feature state, but the ExistentialRequiresAny request does not. Thoughts?

Copy link
Collaborator Author

@AnthonyLatsis AnthonyLatsis Apr 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I was going to but that is what the serializer calls to obtain the evaluation result, which is somewhat misleading in my opinion. I think we should rename the request to something like HasSelfOrAssociatedTypeRequirements and pair it with a method to use in the serializer so that existentialRequiresAny could account for language feature state.

Copy link
Member

Choose a reason for hiding this comment

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

👍 that sounds like a good approach to me (and we don't need to do that as part of this PR)

bool existentialRequiresAny() const;

/// Returns a list of protocol requirements that must be assessed to
Expand Down
4 changes: 0 additions & 4 deletions lib/Sema/TypeCheckDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -714,10 +714,6 @@ ExistentialConformsToSelfRequest::evaluate(Evaluator &evaluator,
bool
ExistentialRequiresAnyRequest::evaluate(Evaluator &evaluator,
ProtocolDecl *decl) const {
auto &ctx = decl->getASTContext();
if (ctx.LangOpts.hasFeature(Feature::ExistentialAny))
return true;

// ObjC protocols do not require `any`.
if (decl->isObjC())
return false;
Expand Down
10 changes: 8 additions & 2 deletions lib/Sema/TypeCheckType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5166,6 +5166,10 @@ class ExistentialTypeVisitor
if (T->isInvalid())
return;

if (Ctx.LangOpts.hasFeature(Feature::ImplicitSome)) {
return;
}

// Compute the type repr to attach 'any' to.
TypeRepr *replaceRepr = T;
// Insert parens in expression context for '(any P).self'
Expand Down Expand Up @@ -5198,7 +5202,8 @@ class ExistentialTypeVisitor
OS << ")";

if (auto *proto = dyn_cast_or_null<ProtocolDecl>(T->getBoundDecl())) {
if (proto->existentialRequiresAny() && !Ctx.LangOpts.hasFeature(Feature::ImplicitSome)) {
if (Ctx.LangOpts.hasFeature(Feature::ExistentialAny) ||
proto->existentialRequiresAny()) {
Ctx.Diags.diagnose(T->getNameLoc(),
diag::existential_requires_any,
proto->getDeclaredInterfaceType(),
Expand All @@ -5216,7 +5221,8 @@ class ExistentialTypeVisitor
if (type->isConstraintType()) {
auto layout = type->getExistentialLayout();
for (auto *protoDecl : layout.getProtocols()) {
if (!protoDecl->existentialRequiresAny() || Ctx.LangOpts.hasFeature(Feature::ImplicitSome))
if (!Ctx.LangOpts.hasFeature(Feature::ExistentialAny) &&
!protoDecl->existentialRequiresAny())
continue;

Ctx.Diags.diagnose(T->getNameLoc(),
Expand Down
2 changes: 1 addition & 1 deletion test/expr/postfix/call/forward_trailing_closure_errors.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ func testUnlabeledParamMatching(i: Int, fn: ((Int) -> Int) -> Void) {

// When "fuzzy matching is disabled, this will fail.
func forwardMatchFailure( // expected-note{{declared here}}
onError: ((Error) -> Void)? = nil,
onError: ((any Error) -> Void)? = nil,
onCompletion: (Int) -> Void
) { }

Expand Down
29 changes: 20 additions & 9 deletions test/type/explicit_existential.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,17 +207,9 @@ extension MyError {
}
}

struct Wrapper {
typealias E = Error
}

func typealiasMemberReferences(metatype: Wrapper.Type) {
let _: Wrapper.E.Protocol = metatype.E.self
let _: (any Wrapper.E).Type = metatype.E.self
}

func testAnyTypeExpr() {
let _: (any P).Type = (any P).self
let _: (any P1 & P2).Type = (any P1 & P2).self

func test(_: (any P).Type) {}
test((any P).self)
Expand Down Expand Up @@ -281,6 +273,12 @@ enum EE : Equatable, any Empty { // expected-error {{raw type 'any Empty' is not
case hack
}

// Protocols from a serialized module (the standard library).
do {
let _: Decodable
let _: Codable
}

func testAnyFixIt() {
struct ConformingType : HasAssoc {
typealias Assoc = Int
Expand All @@ -302,6 +300,19 @@ func testAnyFixIt() {
// expected-error@+2 {{use of protocol 'HasAssoc' as a type must be written 'any HasAssoc'}}{{10-18=(any HasAssoc)}}
// expected-error@+1 {{use of protocol 'HasAssoc' as a type must be written 'any HasAssoc'}}{{30-38=(any HasAssoc)}}
let _: HasAssoc.Protocol = HasAssoc.self
do {
struct Wrapper {
typealias HasAssocAlias = HasAssoc
}
let wrapperMeta: Wrapper.Type
// FIXME: Both of these fix-its are wrong.
// 1. 'any' is attached to 'HasAssocAlias' instead of 'Wrapper.HasAssocAlias'
// 2. What is the correct fix-it for the initializer?
//
// expected-error@+2:20 {{use of 'Wrapper.HasAssocAlias' (aka 'HasAssoc') as a type must be written 'any Wrapper.HasAssocAlias' (aka 'any HasAssoc')}}{{20-33=(any HasAssocAlias)}}
// expected-error@+1:57 {{use of 'Wrapper.HasAssocAlias' (aka 'HasAssoc') as a type must be written 'any Wrapper.HasAssocAlias' (aka 'any HasAssoc')}}{{57-70=(any HasAssocAlias)}}
let _: Wrapper.HasAssocAlias.Protocol = wrapperMeta.HasAssocAlias.self
}
// expected-error@+1 {{use of protocol 'HasAssoc' as a type must be written 'any HasAssoc'}}{{11-19=any HasAssoc}}
let _: (HasAssoc).Protocol = (any HasAssoc).self
// expected-error@+1 {{use of protocol 'HasAssoc' as a type must be written 'any HasAssoc'}}{{10-18=(any HasAssoc)}}
Expand Down
15 changes: 15 additions & 0 deletions test/type/explicit_existential_multimodule1.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// RUN: %empty-directory(%t)
// RUN: %target-swift-frontend %s -swift-version 5 -emit-module -DM -module-name M -emit-module-path %t/M.swiftmodule -enable-upcoming-feature ExistentialAny
// RUN: %target-swift-frontend %s -verify -swift-version 5 -typecheck -I %t

// Test that the feature state used to compile a module is not reflected in
// the module file. The feature must not be enforced on protocols originating in
// a different module just because that module was compiled with the feature
// enabled.

#if M
public protocol P {}
#else
import M
func test(_: P) {}
#endif
15 changes: 15 additions & 0 deletions test/type/explicit_existential_multimodule2.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// RUN: %empty-directory(%t)
// RUN: %target-swift-frontend %s -swift-version 5 -emit-module -DM -module-name M -emit-module-path %t/M.swiftmodule
// RUN: %target-swift-frontend %s -verify -swift-version 5 -typecheck -I %t -enable-upcoming-feature ExistentialAny

// Test that a protocol that requires 'any' *only* when the feature is enabled
// is diagnosed as expected when said protocol originates in a different module.
// In other words, test that deserialization does not affect 'any' migration
// diagnostics.

#if M
public protocol P {}
#else
import M
func test(_: P) {} // expected-error {{use of protocol 'P' as a type must be written 'any P'}}
#endif
30 changes: 21 additions & 9 deletions test/type/explicit_existential_swift6.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,17 +229,9 @@ extension MyError {
}
}

struct Wrapper {
typealias E = Error
}

func typealiasMemberReferences(metatype: Wrapper.Type) {
let _: Wrapper.E.Protocol = metatype.E.self
let _: (any Wrapper.E).Type = metatype.E.self
}

func testAnyTypeExpr() {
let _: (any P).Type = (any P).self
let _: (any P1 & P2).Type = (any P1 & P2).self

func test(_: (any P).Type) {}
test((any P).self)
Expand Down Expand Up @@ -311,6 +303,13 @@ enum EE : Equatable, any Empty { // expected-error {{raw type 'any Empty' is not
case hack
}

// Protocols from a serialized module (the standard library).
do {
// expected-error@+1 {{use of protocol 'Decodable' as a type must be written 'any Decodable'}}
let _: Decodable
// expected-error@+1 2 {{use of 'Codable' (aka 'Decodable & Encodable') as a type must be written 'any Codable' (aka 'any Decodable & Encodable')}}
let _: Codable
}

func testAnyFixIt() {
struct ConformingType : HasAssoc {
Expand All @@ -333,6 +332,19 @@ func testAnyFixIt() {
// expected-error@+2 {{use of protocol 'HasAssoc' as a type must be written 'any HasAssoc'}}{{10-18=(any HasAssoc)}}
// expected-error@+1 {{use of protocol 'HasAssoc' as a type must be written 'any HasAssoc'}}{{30-38=(any HasAssoc)}}
let _: HasAssoc.Protocol = HasAssoc.self
do {
struct Wrapper {
typealias HasAssocAlias = HasAssoc
}
let wrapperMeta: Wrapper.Type
// FIXME: Both of these fix-its are wrong.
// 1. 'any' is attached to 'HasAssocAlias' instead of 'Wrapper.HasAssocAlias'
// 2. What is the correct fix-it for the initializer?
//
// expected-error@+2:20 {{use of 'Wrapper.HasAssocAlias' (aka 'HasAssoc') as a type must be written 'any Wrapper.HasAssocAlias' (aka 'any HasAssoc')}}{{20-33=(any HasAssocAlias)}}
// expected-error@+1:57 {{use of 'Wrapper.HasAssocAlias' (aka 'HasAssoc') as a type must be written 'any Wrapper.HasAssocAlias' (aka 'any HasAssoc')}}{{57-70=(any HasAssocAlias)}}
let _: Wrapper.HasAssocAlias.Protocol = wrapperMeta.HasAssocAlias.self
}
// expected-error@+1 {{use of protocol 'HasAssoc' as a type must be written 'any HasAssoc'}}{{11-19=any HasAssoc}}
let _: (HasAssoc).Protocol = (any HasAssoc).self
// expected-error@+1 {{use of protocol 'HasAssoc' as a type must be written 'any HasAssoc'}}{{10-18=(any HasAssoc)}}
Expand Down