Skip to content

Conversation

johnno1962
Copy link
Contributor

Hi Apple,

This is a follow-up to #25786 attempting to remove the constraint that extensions of protocols cannot have conformances. It's in reasonably good shape working across modules, not involving major surgery on the compiler and working well enough to be able to build a toolchain. Only a handful of tests are broken at the moment. Most of the implementation is localised to the source file ConformanceLookupTable.cpp with small targeted changes elsewhere as required. It works by recursively resolving conformances each time the conformances of a nominal are looked up and tracking which inferred (implied?) conformances are used so they can trigger the generation of witness tables. As multiple source files can force the generation of a witness table they are emitted private to the file to avoid duplicate symbols. This results in a certain amount of duplication but witness tables don't occupy much memory and is less of a problem if you use whole module optimisation for your build. The Swift runtime doesn't seem to mind.

I'm filing this PR now in order to get some initial feedback and I'll be posting to swift evolution to gather some more test code to see if people can break the implementation as is. Compiler crashes are not unheard of and error messages could be better but if a program compiles I haven't seen any misbehaviour at run time as yet. I hope to be able to demonstrate it is viable to implement this functionality mentioned in the generics manifesto and I'd personally very much like to see this as a part of the Swift language (subject to review).

There is a toolchain available here to evaluate.

Addresses https://bugs.swift.org/browse/SR-11013.

@johnno1962 johnno1962 force-pushed the protocol-extension-conformances branch from 1f34050 to 7d659bf Compare January 17, 2020 14:49
@johnno1962
Copy link
Contributor Author

Could someone build a Linux toolchain for me please so I can test this on 18.04. @harlanhaskins?

@harlanhaskins
Copy link
Contributor

@swift-ci please build toolchain Linux platform

@theblixguy
Copy link
Collaborator

Its also worth running a performance test (because that was one of the reasons given in the manifesto).

@johnno1962
Copy link
Contributor Author

johnno1962 commented Jan 17, 2020

Benchmarks for compilation and runtime speed should be run but it shouldn’t be a problem at runtime as the witnesses would be determined at compile time (with a small amount of extra work and only if the feature is being used.)

@swift-ci
Copy link
Contributor

Linux Toolchain (Ubuntu 16.04)
Download Toolchain
Git Sha - 7d659bf72a5b117909ab96e9e114b0af20734ee1

Install command
tar zxf swift-PR-29272-342-ubuntu16.04.tar.gz
More info

@johnno1962
Copy link
Contributor Author

Thanks @harlanhaskins, that saved me a lot of time. The example project https://github.com/johnno1962/ExtApp checks out with SPM on Linux.

Copy link
Contributor

Choose a reason for hiding this comment

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

This cannot be a warning. This needs to remain an error that goes away when the flag is enabled.

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 can easily be changed if this ever nears being merged. I left it was a warning for testing while getting the code for gating the feature in place. Thanks for taking a look!

@johnno1962 johnno1962 force-pushed the protocol-extension-conformances branch 2 times, most recently from 337d8bf to a24f00c Compare January 27, 2020 15:16

Choose a reason for hiding this comment

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

Suggested change
/// Iterate over conformances conformances arising from protocol extensions
/// Iterate over conformances arising from protocol extensions

Copy link
Contributor Author

@johnno1962 johnno1962 Jan 27, 2020

Choose a reason for hiding this comment

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

Oops, one “conformances" is probably enough. Thanks.

@johnno1962 johnno1962 force-pushed the protocol-extension-conformances branch from a24f00c to 8341f53 Compare January 27, 2020 16:29
@johnno1962 johnno1962 force-pushed the protocol-extension-conformances branch from 7c1fd48 to 556b4df Compare February 12, 2020 09:09
@johnno1962
Copy link
Contributor Author

johnno1962 commented Feb 12, 2020

Made a bit more progress on this PR and might have found a solution to the “witness table stability” problem by moving conformances derived from a protocol extension to the very end of the witness table (after the function witnesses). In this way a module that was not have the extension can still use the witness. Down to edge cases that crop up between modules now, it even works when you extend to acquire protocols that synthesize somehow 🤠. @slavapestov, do you have time for a quick look over the code and point me towards what might be other potential problems to resolve?

To give you a bit more to go on there are two main areas where I had to made changes - the ConformanceLookupTable.cpp in the way it recurses through conformances and the GenericSignatureBuilder.cpp to cater for protocols being extendable. The conformance table tracks the extra witness tables that have to be emitted though there may be a better way to do this on demand in the long run. It’s not entirely clear what is the interaction with resilience and I’ve commented out a couple assertions in the bliss of ignorance for now. Finally the SILWitnessVisitor ensures new protocol witnesses are added at the end and the approach becomes viable. The protocol witnesses are added in the order the extensions are encountered so if if a number of modules extend they should be able to cascade. The new conformances for nominals are local to the object file to avoid duplicate symbols so they shouldn’t interfere.

Toolchain available here: http://johnholdsworth.com/swift-LOCAL-2020-02-11-a-osx.tar.gz.

@johnno1962 johnno1962 force-pushed the protocol-extension-conformances branch 9 times, most recently from cc2f993 to 22bd817 Compare February 16, 2020 01:09
@johnno1962 johnno1962 force-pushed the protocol-extension-conformances branch from 22bd817 to 1d09313 Compare March 23, 2020 10:21
@johnno1962
Copy link
Contributor Author

johnno1962 commented Apr 1, 2020

A progress report of sorts on this experiment. It is possible to get something working inside a module but, trying to define a type/protocol in one module and extend it’s conformances by extending the conformances of a protocol in another module the wheels fall off and you can find Swift trying to read function/protocol pointers off the end of the witness table defined in the original module and I don’t see how this can be avoided at the moment. So, in the finish you end up with a feature subject to subtle and arbitrary limitations which are only discoverable at run time. “They said it couldn’t be done” and it seems like they were right 😞.

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.

6 participants