Skip to content

[concurrency] Implement effectful properties #36430

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

Jump to bottom
Merged
merged 13 commits into from
Mar 26, 2021
Merged

[concurrency] Implement effectful properties #36430

merged 13 commits into from
Mar 26, 2021

Conversation

kavon
Copy link
Member

@kavon kavon commented Mar 13, 2021 •
edited
Loading

https://forums.swift.org/t/pitch-effectful-read-only-properties/44090

TODOs:

  • Hide behind -Xfrontend -enable-experimental-concurrency flag
  • Parsing / serialization / deserialization
    • Implementation
    • Test parsing
    • Test serialization/deserialization.
  • Typechecking
    • async / throws read access sites & their coverage
    • Protocols (conformance, composition, inheritance)
    • Class Inheritance & Overrides
    • Actor Isolation
    • Key Paths
    • Prevent annotation with @objc (causes crashes in typechecking, for good reason)
    • Make sure protocol test covers subscripts
    • Raise error about effectful props in property wrappers
  • Code generation
    • Test SILGen
    • Executable test

Deferred Tasks:

  • Property Wrappers
  • A useful error message when an attempt is made to use effects in an implicit getter.
  • Overload resolution issue in this code, where the u.someProp from A should be chosen due to being in an async context:
protocol None {
  associatedtype V
  var someProp : V { get }
}
protocol A {
  associatedtype V
  var someProp : V { get async }
}
func f<U : A & None>(u : U) async {
  _ = u.someProp // no errors raised here!
}

Note that the checker disallows conformance to A & None with an async property witness, so this does not ultimately lead to a mis-compile. It is, however, a bit misleading to allow someone to write f as it stands, and doesn't match up with how we handle this for functions.

@kavon kavon changed the title Implement effectful properties [concurrency] Implement effectful properties Mar 13, 2021
@kavon kavon force-pushed the effectful-properties branch 7 times, most recently from eac2332 to 7a418ab Compare March 22, 2021 17:28
@kavon kavon force-pushed the effectful-properties branch 7 times, most recently from c43a4bc to 3a1d7a0 Compare March 25, 2021 00:59
@kavon kavon marked this pull request as ready for review March 25, 2021 01:00
@kavon
Copy link
Member Author

kavon commented Mar 25, 2021

@swift-ci please smoke test

ktoso
ktoso reviewed Mar 25, 2021
View reviewed changes
ktoso
ktoso reviewed Mar 25, 2021
View reviewed changes
@kavon kavon force-pushed the effectful-properties branch 2 times, most recently from 5e5dac7 to 9f36726 Compare March 25, 2021 16:37
@kavon
Copy link
Member Author

kavon commented Mar 25, 2021

@swift-ci please smoke test

@kavon kavon force-pushed the effectful-properties branch from 9f36726 to e3fc0d7 Compare March 25, 2021 18:51
@kavon
Copy link
Member Author

kavon commented Mar 25, 2021

@swift-ci please smoke test and merge

@kavon kavon force-pushed the effectful-properties branch from e3fc0d7 to d28e031 Compare March 25, 2021 22:34
@kavon
Copy link
Member Author

kavon commented Mar 25, 2021

@swift-ci please smoke test and merge

DougGregor
DougGregor approved these changes Mar 26, 2021
View reviewed changes
Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

I read through this and don't have any substantial comments, which means I should read through it again... but that shouldn't block merging.

@kavon kavon force-pushed the effectful-properties branch from d28e031 to 4d248bb Compare March 26, 2021 01:50
@kavon
Copy link
Member Author

kavon commented Mar 26, 2021

@swift-ci please smoke test and merge

@kavon
Copy link
Member Author

kavon commented Mar 26, 2021

This is the PR that I learn about // REQUIRES: objc_interop :)

@kavon kavon force-pushed the effectful-properties branch from 4d248bb to 68e50f9 Compare March 26, 2021 02:53
@kavon
Copy link
Member Author

kavon commented Mar 26, 2021

@swift-ci please smoke test and merge

@kavon kavon force-pushed the effectful-properties branch from 68e50f9 to 292b927 Compare March 26, 2021 04:35
@kavon
Copy link
Member Author

kavon commented Mar 26, 2021

@swift-ci please smoke test and merge

@DougGregor
Copy link
Member

Just the driver failure on Linux... will try again because that should be fixed by now.

@DougGregor
Copy link
Member

@swift-ci please smoke test Linux

kavon added 13 commits March 26, 2021 07:54
@kavon
implement parsing for effects specifiers on 'get' accessors
f9452b2 
An effectful 'get' accessor must be the only accessor for the
property.
@kavon
[test] update regression test for effectful props
7825e37 
With effectful properties, we'll now raise an error if
an effect specifier appears on a non-'get' accessor.
@kavon
[test] parsing for effectful properties
2a4c036
@kavon
[test] AST serialization for effectful properties
3702d0c
@kavon
implement typechecking for effects specifiers on 'get' accessors
efbb0f1 
The basic rule is that the protocol requirement
describes the maximal set of effects that the
property is allowed to have. Thus, witnesses
must have the same or fewer effects specifiers
on the getter.

For class inheritance overrides, you can remove
effects freely, as long as you stay within
the bounds of the normal override restrictions.
But, you cannot override with more effects than
the base member has. Same goes for protocol
member overrides.

Furthermore, we disallow key paths to effectful
properties/subscripts, which also cannot be @objc.
@kavon
[test] eff. prop + actor isolation
f3be40e
@kavon
[test] eff. prop + protocol conformance, composition & inheritance
47ea602
@kavon
[test] eff. prop + class inheritance & overrides
e418403
@kavon
[test] coverage for key paths to effectful prop/subscript
494a689
@kavon
[test] effectful properties through SILGen
9b09d8a
@kavon
[test] effectful properties + property wrappers
e943445 
They're currently incompatible, but it should be
possible to enable this, with some care taken to
ensure that effectful wrappers are composed
correctly.
@kavon
[test] coverage for global effectful props
7585f46
@kavon
[test] execution test for effectful properties
8ba98a9
@kavon kavon force-pushed the effectful-properties branch from 292b927 to 8ba98a9 Compare March 26, 2021 14:58
@kavon
Copy link
Member Author

kavon commented Mar 26, 2021

OK I rebased and fixed the change to the asyncHandler note. Rerunning test locally before triggering another run

@kavon
Copy link
Member Author

kavon commented Mar 26, 2021

@swift-ci please smoke test and merge

@swift-ci swift-ci merged commit bae76c8 into swiftlang:main Mar 26, 2021
@kavon kavon deleted the effectful-properties branch March 26, 2021 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ktoso ktoso ktoso left review comments

@DougGregor DougGregor DougGregor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kavon @DougGregor @ktoso @swift-ci