Allow protocols to be nested in non-generic contexts #66247

karwa · 2023-05-31T14:40:11Z

Allows protocols to be nested in non-generic types/functions.

Gated behind flag -enable-experimental-feature NestedProtocols. SE proposal coming soon.

TODO:

Test that ASTPrinter adds feature guards around uses of nested protocols
IRGen tests

hborla · 2023-05-31T15:27:42Z

lib/AST/ASTPrinter.cpp

@@ -3425,6 +3425,14 @@ static bool usesFeatureParameterPacks(Decl *decl) {
  return false;
 }

+static bool usesFeatureNestedProtocols(Decl *decl) {
+    // TODO: How do I test this?


You can test this through swfitinterface printing. Check out test/ModuleInterface/pack_expansion_type.swift as an example, which exercises usesFeatureParameterPacks and checks for #if guards around declarations that use parameter packs.

lib/Sema/TypeCheckDeclPrimary.cpp

hborla · 2023-05-31T18:47:23Z

Cool! It looks like you're still actively working on this, so let me know if/when you'd like to kick off CI, e.g. to build a toolchain

karwa · 2023-05-31T19:49:30Z

So, one thing that is interesting is the module interface printing - the logic in the PR is a bit too simplistic; if the goal is to print an interface which can be successfully parsed by an older compiler (using feature guards), then it's not enough to only guard the protocol declarations themselves -- it also needs to guard any use of a nested protocol (e.g. in a function's parameters or return type, or within a generic signature). And I think that also extends to typealiases which are bound to nested protocols, across translation units, etc. So it's kind of complex.

What is the best way to approach that? Can it even be automated, or will users have to guard those uses manually using #if swift(>=...) checks?

slavapestov · 2023-05-31T20:26:57Z

then it's not enough to only guard the protocol declarations themselves -- it also needs to guard any use of a nested protocol (e.g. in a function's parameters or return type, or within a generic signature).

Now imagine a declaration that transitively depends on a declaration that uses a nested protocol; it would also need to be feature-guarded, if you were going for an airtight implementation.

What is the best way to approach that?

I would simply punt on the issue. Feature guards are only intended to solve a limited set of problems with staging in toolchain/stdlib/SDK changes and we can't really hope to always generate an interface file that remains parseable by older compilers.

slavapestov · 2023-05-31T22:40:40Z

test/decl/nested/protocol_experimental.swift

@@ -0,0 +1,261 @@
+// RUN: %target-typecheck-verify-swift -parse-as-library -enable-experimental-feature NestedProtocols


Please also add some IRGen tests to cover emission of protocol descriptors, and execution tests. In particular, I'm curious if a dynamic cast to a conditional conformance involving one of these things will work immediately, or require runtime changes:

enum E { protocol P {} } struct S<T> {} extension S: E.P where T: E.P {} extension Int: E.P {} let x: Any = S<Int>() print(x as? any E.P)

karwa · 2023-06-02T08:06:11Z

@slavapestov

I would simply punt on the issue. Feature guards are only intended to solve a limited set of problems with staging in toolchain/stdlib/SDK changes and we can't really hope to always generate an interface file that remains parseable by older compilers.

OK. I thought about maybe hiding just the protocol type, but the type declaration will almost always live alongside some uses of the protocol so it probably wouldn't be very useful in real projects, and in the worst case an older compiler could end up with an incorrect interface:

protocol Foo { ... }

enum MyEnum {
  #if compiler(>=5.3) && $NestedProtocols
    protocol Foo { ... }
  #endif

  func doSomething<T: Foo>(_: T)
  // ^ Argument may be top-level Foo or nested Foo depending on compiler version!
}

Probably (hopefully) that would end in a linker failure, but it's still not good. Maybe type declarations should not be guarded in general because of this. So yeah, in summary - I'll remove the feature guard in the module interface and add a test to ensure it isn't printed.

In particular, I'm curious if a dynamic cast to a conditional conformance involving one of these things will work immediately, or require runtime changes

Your example works for me using this patch. I will add an executable test which includes it (as well as the IRGen descriptor tests).

slavapestov · 2023-06-02T17:09:00Z

A couple more things I thought of:

we probably want to ban @objc protocols from being nested
we should test code generation and execution for protocols inside functions
we should test binary serialization and deserialization

karwa · 2023-06-09T10:48:21Z

we probably want to ban @objc protocols from being nested

Why?

I mean, there is an issue that we don't namespace generated Obj-C names, but that is equally a problem for nested classes. For example, see the test PrintAsObjC/classes. It takes the Swift interface:

@objc @objcMembers class Nested {
  @objc @objcMembers class Inner {
    @objc @objcMembers class DeeperIn {}
  }
}

And generates the following Obj-C header. Note that the SWIFT_CLASS mangled names are correct (so that's a way to discriminate between Foo.Inner and Bar.Inner), but if somebody were to use the non-mangled name Inner in their Obj-C code, how would the Obj-C compiler know which class was intended?

SWIFT_CLASS("_TtC7classes6Nested")
@interface Nested
- (nonnull instancetype)init OBJC_DESIGNATED_INITIALIZER;
@end


SWIFT_CLASS("_TtCC7classes6Nested5Inner")
@interface Inner
- (nonnull instancetype)init OBJC_DESIGNATED_INITIALIZER;
@end


SWIFT_CLASS("_TtCCC7classes6Nested5Inner8DeeperIn")
@interface DeeperIn
- (nonnull instancetype)init OBJC_DESIGNATED_INITIALIZER;
@end

So clearly that's a problem, but it is not uniquely a problem for protocols and deserves a comprehensive solution. That solution would be source-breaking, and it's not going to be worse for protocols than any other nested type.

Evidently, some people seem to be able to work with the current limitations, and it's not clear to me that they should be prohibited from exposing nested protocols. They may wish to use a custom Obj-C name as a workaround, while using nesting for the Swift version of the interface.

slavapestov · 2023-06-09T13:30:56Z

I mean, there is an issue that we don't namespace generated Obj-C names, but that is equally a problem for nested classes. For example, see the test PrintAsObjC/classes. It takes the Swift interface:

Good point. Let's just add some test cases then.

karwa · 2023-06-09T14:50:53Z

@slavapestov OK I've added:

Refinements for the diagnostic
Tests for PrintAsObjC (including a test for a nested classes with a custom Obj-C name, which is something I think we can fix without breaking source)
Test that IRGen emits protocol descriptors. I didn't actually base this on my old PR; I figured the compiler had moved on a lot and I wanted to rediscover what was needed so I had a fresh understanding, and I discovered (again) that if parseDeclProtocol doesn't set the local discriminator, we wouldn't emit protocol descriptors for function-local protocols. I checked what happens if I revert that change, and this test will catch it.

Still todo:

ModuleInterface test (there should be no guarding of nested protocols)
Serialisation/deserialisation test
Executable test

And I think that's it, right? Could you run CI tests on what I have so far, just to confirm it works like it does for me locally?

slavapestov · 2023-06-09T15:24:08Z

@swift-ci Please test

slavapestov · 2023-06-09T15:24:14Z

@swift-ci Please test source compatibility

hborla · 2023-07-23T22:40:24Z

@swift-ci please build toolchain

hborla · 2023-07-23T22:41:41Z

@karwa I resolved conflicts via merge because I didn't want to force-push to your branch, but please feel free to redo it as a rebase if you'd like.

slavapestov

I just have a couple more minor suggestions.

lib/Sema/TypeCheckDeclPrimary.cpp

lib/AST/DeclContext.cpp

karwa · 2023-10-06T15:40:28Z

@slavapestov Implemented the change you suggested. Could you trigger CI please?

slavapestov · 2023-10-06T16:14:01Z

test/decl/nested/protocol.swift

-  // expected-error@-1 {{protocol 'P' cannot be nested inside another declaration}}
+struct NestedInGenericMethod<T> {
+  func someMethod() {
+    protocol AnotherTest {} // expected-error{{type 'AnotherTest' cannot be nested in generic function 'someMethod()'}}


Shouldn't this use the "protocol %0 cannot be nested in a generic context" diagnostic?

Another change (that I rebased in) guarded these checks inside if (auto *parentDecl = DC->getSelfNominalTypeDecl()). In the function, getSelfNominalTypeDecl returns null, so this goes along the regular nominal-in-generic-context error path. That path gives nicer diagnostics, so I left it that way.

slavapestov · 2023-10-06T16:14:19Z

@swift-ci Please smoke test

…th inferred params

karwa · 2023-10-06T19:06:22Z

Test failure is due to actor availability. Doesn't happen locally when I run the tests on Sonoma. I guess it happens because CI is using an ancient version of macOS?

Command Output (stderr):
--
/Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/swift/test/decl/nested/protocol.swift:52:13: error: unexpected error produced: concurrency is only available in macOS 10.15.0 or newer
      actor Level3 {
            ^
/Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/swift/test/decl/nested/protocol.swift:52:13: error: unexpected note produced: add @available attribute to enclosing actor
      actor Level3 {
            ^

Fixed by changing that actor be an enum. Added an additional test for nesting a protocol in an actor with an availability annotation.

slavapestov · 2023-10-06T19:44:22Z

I guess it happens because CI is using an ancient version of macOS?

I believe it's about the default deployment target in the SDK, but I forget the details.

Fixed by changing that actor be an enum. Added an additional test for nesting a protocol in an actor with an availability annotation.

It is also fine to pass -disable-availability-checking in tests like this.

slavapestov · 2023-10-06T19:44:27Z

@swift-ci Please smoke test

slavapestov · 2023-10-07T01:51:28Z

/home/build-user/build/buildbot_linux/swift-linux-x86_64/test-linux-x86_64/Serialization/Output/nested-protocols.swift.script: line 3: Filecheck: command not found

Looks like this test fails on a case-sensitive file system.

karwa · 2023-10-07T03:36:24Z

@slavapestov Oops. But yeah, the macOS filesystem seems to be case-insensitive by default, so that didn't happen locally. At least that should be the last thing, since the macOS test was okay.

slavapestov · 2023-10-07T15:12:29Z

@swift-ci Please smoke test

karwa · 2023-10-07T16:30:26Z

Hmm it seems I need to use the substitution %FileCheck for FileCheck.

For some reason that also wasn't necessary on macOS.

slavapestov · 2023-10-07T17:43:03Z

@swift-ci Please smoke test

slavapestov · 2023-10-08T16:01:49Z

@karwa Great job with the PR. I merged it since I don't think there is anything else outstanding here. Also, the proposal was accepted, so I think you can remove the experimental feature flag now.

slavapestov · 2023-10-08T16:04:01Z

Also do you want to cherry-pick this to the release/5.10 branch?

karwa · 2023-10-09T10:37:28Z

Thanks @slavapestov! And thanks for your guidance along the way!

Yeah I'll cherry-pick this to the 5.10 branch :)

karwa requested review from hborla, slavapestov, xedin, ahoppen, bnbarham, CodaFi, DougGregor, rintaro and tshortli as code owners May 31, 2023 14:40

karwa force-pushed the nested-protocols branch from 2ef9f8a to 86783f0 Compare May 31, 2023 14:48

hborla reviewed May 31, 2023

View reviewed changes

ahoppen removed their request for review May 31, 2023 17:19

hborla reviewed May 31, 2023

View reviewed changes

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

slavapestov reviewed May 31, 2023

View reviewed changes

karwa force-pushed the nested-protocols branch from 3bf6543 to d8282c4 Compare June 9, 2023 19:17

karwa force-pushed the nested-protocols branch from 9f8e6de to 9cc80a2 Compare October 5, 2023 11:13

slavapestov reviewed Oct 5, 2023

View reviewed changes

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

lib/AST/DeclContext.cpp Show resolved Hide resolved

karwa force-pushed the nested-protocols branch from 9cc80a2 to f3a4558 Compare October 6, 2023 15:38

slavapestov approved these changes Oct 6, 2023

View reviewed changes

karwa added 3 commits October 6, 2023 21:04

[SE-0404] Allow protocols to be nested in non-generic contexts

ab4f80e

[SE-0404] Rebase fixes and better handling of generic superclasses wi…

69c5704

…th inferred params

[SE-0404] Make test for protocol nested in actor conditionally-available

853d3c8

karwa force-pushed the nested-protocols branch from 4f81f3d to 853d3c8 Compare October 6, 2023 19:05

[Test] Fix use of 'FileCheck'

f71d7b1

karwa force-pushed the nested-protocols branch from 4b98f6c to f71d7b1 Compare October 7, 2023 16:27

slavapestov merged commit f321dd3 into swiftlang:main Oct 8, 2023
3 checks passed

karwa mentioned this pull request Oct 10, 2023

[5.10][SE-0404] Allow protocols to be nested in non-generic contexts #69103

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow protocols to be nested in non-generic contexts #66247

Allow protocols to be nested in non-generic contexts #66247

karwa commented May 31, 2023

hborla May 31, 2023

hborla commented May 31, 2023

karwa commented May 31, 2023 •

edited

Loading

slavapestov commented May 31, 2023

slavapestov May 31, 2023

karwa commented Jun 2, 2023

slavapestov commented Jun 2, 2023

karwa commented Jun 9, 2023

slavapestov commented Jun 9, 2023

karwa commented Jun 9, 2023 •

edited

Loading

slavapestov commented Jun 9, 2023

slavapestov commented Jun 9, 2023

hborla commented Jul 23, 2023

hborla commented Jul 23, 2023

slavapestov left a comment

karwa commented Oct 6, 2023

slavapestov Oct 6, 2023

karwa Oct 6, 2023

slavapestov commented Oct 6, 2023

karwa commented Oct 6, 2023

slavapestov commented Oct 6, 2023

slavapestov commented Oct 6, 2023

slavapestov commented Oct 7, 2023

karwa commented Oct 7, 2023

slavapestov commented Oct 7, 2023

karwa commented Oct 7, 2023 •

edited

Loading

slavapestov commented Oct 7, 2023

slavapestov commented Oct 8, 2023

slavapestov commented Oct 8, 2023

karwa commented Oct 9, 2023

		@@ -0,0 +1,261 @@
		// RUN: %target-typecheck-verify-swift -parse-as-library -enable-experimental-feature NestedProtocols

Allow protocols to be nested in non-generic contexts #66247

Allow protocols to be nested in non-generic contexts #66247

Conversation

karwa commented May 31, 2023

hborla May 31, 2023

Choose a reason for hiding this comment

hborla commented May 31, 2023

karwa commented May 31, 2023 • edited Loading

slavapestov commented May 31, 2023

slavapestov May 31, 2023

Choose a reason for hiding this comment

karwa commented Jun 2, 2023

slavapestov commented Jun 2, 2023

karwa commented Jun 9, 2023

slavapestov commented Jun 9, 2023

karwa commented Jun 9, 2023 • edited Loading

slavapestov commented Jun 9, 2023

slavapestov commented Jun 9, 2023

hborla commented Jul 23, 2023

hborla commented Jul 23, 2023

slavapestov left a comment

Choose a reason for hiding this comment

karwa commented Oct 6, 2023

slavapestov Oct 6, 2023

Choose a reason for hiding this comment

karwa Oct 6, 2023

Choose a reason for hiding this comment

slavapestov commented Oct 6, 2023

karwa commented Oct 6, 2023

slavapestov commented Oct 6, 2023

slavapestov commented Oct 6, 2023

slavapestov commented Oct 7, 2023

karwa commented Oct 7, 2023

slavapestov commented Oct 7, 2023

karwa commented Oct 7, 2023 • edited Loading

slavapestov commented Oct 7, 2023

slavapestov commented Oct 8, 2023

slavapestov commented Oct 8, 2023

karwa commented Oct 9, 2023

karwa commented May 31, 2023 •

edited

Loading

karwa commented Jun 9, 2023 •

edited

Loading

karwa commented Oct 7, 2023 •

edited

Loading