[Property Wrappers] Support local property wrappers #34109

hborla · 2020-09-28T23:45:41Z

Support property wrappers on local variables, e.g.

@propertyWrapper
struct Wrapper<T> {
  var wrappedValue: T
}

func test() {
  @Wrapper var value = 10
}

has a custom attribute.

hborla · 2020-09-28T23:45:53Z

@swift-ci please test source compatibility

…pper and projected value references

…nside an initializer, and for wrapped local variables.

hborla · 2020-09-28T23:59:51Z

@swift-ci please test source compatibility

…roperty wrapper accessors.

property wrapper vars.

slavapestov · 2020-09-29T19:40:57Z

lib/Sema/TypeCheckStorage.cpp

    body.push_back(returnStmt);
  }

+  // Don't mark local accessors as type-checked - captures still need to be computed.


This seems a bit fragile because you're assuming that type checking is idempotent on already-checked AST. Would it be better to insert an explicit call to computeCaptures() inside TypeCheckDeclPrimary.cpp?

(And yeah, computeCaptures() should be a request, since there are a few places we have to trigger it manually for synthesized code...)

I tried to change this, but I discovered that "re" type checking the accessor is doing more necessary things than just computing captures.

For example, if the wrappedValue getter of a local property wrapper is mutating, buildStorageReference will generate something like for the local getter:

(member_ref_expr implicit type='String' decl=test.(file).Lazy.wrappedValue@test.swift:10:7 [with (substitution_map generic_signature=<Value> (substitution Value -> String))] (declref_expr implicit type='Lazy<String>' decl=test.(file).testLocalLazy()._value@test.swift:28:13 direct_to_storage function_ref=unapplied))

But it really needs to look like this:

(load_expr implicit type='String' (member_ref_expr implicit type='@lvalue String' decl=test.(file).Lazy.wrappedValue@test.swift:10:7 [with (substitution_map generic_signature=<Value> (substitution Value -> String))] (inout_expr implicit type='inout Lazy<String>' (declref_expr implicit type='@lvalue Lazy<String>' decl=test.(file).testLocalLazy()._value@test.swift:28:13 direct_to_storage function_ref=unapplied))))

CSApply knows how to do this, but buildStorageReference doesn't. Is there a reason why buildStorageReference is attempting to build a type-checked expression rather than relying on the constraint system to fill in types?

Addressing this in a follow-up PR is fine. I suggest investigating why the mutating getter case works for property wrappers on members of types but not with local variables. It might be an easy fix.

include/swift/AST/NameLookup.h

lib/SIL/IR/SILDeclRef.cpp

checks to check for type context instead of local context. This generalization will help us implement property wrappers on global variables, which should use the same approach of not adding synthesized accessors to the AST and instead lazily visit them in SILGen.

slavapestov

This looks good to go! I know you haven't written SILGen tests yet but I don't expect to have anything to say about those.

One more thing you might want to do in a follow-up PR is visit auxiliary decls in a couple more places, in case you weren't already planning on it:

LookupVisibleDecls, for code completion and typo correction. Eventually LookupVisibleDecls will use ASTScope, but for now it has a duplicate implementation of unqualified lookup
SourceEntityWalker for indexing

lib/AST/UnqualifiedLookup.cpp

unqualified lookup of auxiliary decl names.

captured local variables for the assign_by_wrapper setter. Since assign_by_wrapper will always be re-written to initialization if the captured local variable is uninitialized, it's unnecessary to mark the capture as an escape. This lets us support out-of-line initialization for local property wrappers.

theblixguy · 2020-09-30T20:45:56Z

lib/AST/Decl.cpp

+
+  if (auto *projectionVar = getPropertyWrapperProjectionVar())
+    visit(projectionVar);
+}


Should this visit the lazy backing storage as well?

Yes! I was going to try to implement local lazy in a followup PR

(unless somebody else gets to it first 🙂 )

Awesome! There’s an old WIP PR for it: #27600 which you may find helpful.

Ah, that one got dropped on the floor because I ended up focusing entirely on figuring out a proper way to insert synthesized decls into a BraceStmt post-factum. They simply get visited here instead, right? I am sort of ashamed of myself for having missed the point, but that's awesome; thanks, Holly!

Yes, that's right - with this approach, I've added calls to visitAuxiliaryDecls in name lookup, the decl checker, and SILGen, so the synthesized VarDecls don't need to be added to the BraceStmt, but they're still visited in the appropriate places.

hborla · 2020-09-30T20:57:18Z

@slavapestov could you also take a look at 4bb98ba ? I want to make sure it's not a terrible idea

hborla · 2020-09-30T21:06:40Z

@swift-ci please smoke test

hborla · 2020-09-30T21:06:49Z

@swift-ci please test source compatibility

hborla · 2020-10-01T16:44:01Z

Doesn't look like there were actually any failures in the source compat suite

just like we do for regular non-member properties.

there is an initial wrapped value.

hborla · 2020-10-02T03:14:29Z

@swift-ci please smoke test

[Parser] Don't resolve decl references in the parser if the declaration

3a47087

has a custom attribute.

hborla added 3 commits September 28, 2020 16:57

[NameLookup] Teach unqualified lookup to resolve backing property wra…

9b2cd5e

…pper and projected value references

[SILGen] Teach SIlGen to emit local property wrappers

b33dbed

[SILGen] Only use assign_by_wrapper for wrapped instance properties i…

0842b42

…nside an initializer, and for wrapped local variables.

hborla force-pushed the local-property-wrappers branch from 00820e2 to 0842b42 Compare September 28, 2020 23:58

hborla added 3 commits September 28, 2020 18:13

[Property Wrappers] Make sure captures are computed for synthesized p…

9bd3d0b

…roperty wrapper accessors.

[Property Wrappers] Allow property wrappers on local variables.

d8df621

[Property Wrappers] Add a VarDecl helper method for visiting synthesized

21cbdfa

property wrapper vars.

BasThomas mentioned this pull request Sep 29, 2020

[170] Issue #170 - October 8, 2020 SwiftWeekly/swiftweekly.github.io#548

Closed

slavapestov reviewed Sep 29, 2020

View reviewed changes

slavapestov approved these changes Sep 29, 2020

View reviewed changes

lib/AST/UnqualifiedLookup.cpp Outdated Show resolved Hide resolved

hborla added 2 commits September 30, 2020 10:40

[Name Lookup] Remove property wrapper name lookup flags and generalize

9e373a2

unqualified lookup of auxiliary decl names.

hborla force-pushed the local-property-wrappers branch from 1d69a1e to 4bb98ba Compare September 30, 2020 20:36

theblixguy reviewed Sep 30, 2020

View reviewed changes

[Test] Start to add SILGen tests for local property wrappers.

176391f

hborla marked this pull request as ready for review September 30, 2020 21:00

hborla added 2 commits October 1, 2020 19:34

[Property Wrappers] Ban non-member property wrappers with observers,

af71c5d

just like we do for regular non-member properties.

[SILGen] Don't use assign_by_wrapper for local property wrappers if

763bcf9

there is an initial wrapped value.

hborla merged commit 355fbb3 into swiftlang:main Oct 2, 2020

hborla deleted the local-property-wrappers branch October 2, 2020 05:45

hborla mentioned this pull request Oct 7, 2020

[Sema] Correct lvalue computation in buildStorageRef for synthesizing local accessors. #34230

Merged

[Property Wrappers] Support local property wrappers #34109

[Property Wrappers] Support local property wrappers #34109

Uh oh!

Conversation

hborla commented Sep 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hborla commented Sep 28, 2020

Uh oh!

hborla commented Sep 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

slavapestov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnthonyLatsis Sep 30, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

hborla commented Sep 30, 2020

Uh oh!

hborla commented Sep 30, 2020

Uh oh!

hborla commented Sep 30, 2020

Uh oh!

hborla commented Oct 1, 2020

Uh oh!

hborla commented Oct 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hborla commented Sep 28, 2020 •

edited

Loading

AnthonyLatsis Sep 30, 2020 •

edited

Loading