Skip to content

[Typechecker] Fix _modify for properties using a property wrapper #28216

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 7 commits into from
Nov 21, 2019
Merged

[Typechecker] Fix _modify for properties using a property wrapper #28216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
47ba176
[Typechecker] Fix _modify for properties using a property wrapper
theblixguy Nov 13, 2019
42473e3
[Test] Update test to check for _read coroutine as well
theblixguy Nov 13, 2019
7244f6c
[Typechecker] Synthesize _modify for storage wrapper correctly
theblixguy Nov 13, 2019
0cd5906
[Test] Update a few failing tests
theblixguy Nov 14, 2019
33be7a2
[Typechecker] Don't use PropertyWrapperMutability for isSelfLValue check
theblixguy Nov 15, 2019
1264c56
Revert "[Typechecker] Don't use PropertyWrapperMutability for isSelfL…
theblixguy Nov 16, 2019
6d96447
[Typechecker] PropertyWrapperMutabilityRequest should not return None…
theblixguy Nov 20, 2019
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
37 changes: 29 additions & 8 deletions lib/Sema/TypeCheckStorage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,8 +743,6 @@ static Expr *buildStorageReference(AccessorDecl *accessor,
bool isMemberLValue = isLValue;
auto propertyWrapperMutability =
[&](Decl *decl) -> Optional<std::pair<bool, bool>> {
if (accessor->isCoroutine())
return None;
auto var = dyn_cast<VarDecl>(decl);
if (!var)
return None;
Expand Down Expand Up @@ -1527,7 +1525,7 @@ synthesizeSetterBody(AccessorDecl *setter, ASTContext &ctx) {
return synthesizePropertyWrapperSetterBody(setter, ctx);
}

// Synthesize a getter for the storage wrapper property of a property
// Synthesize a setter for the storage wrapper property of a property
// with an attached wrapper.
if (auto original = var->getOriginalWrappedProperty(
PropertyWrapperSynthesizedPropertyKind::StorageWrapper)) {
Expand Down Expand Up @@ -1572,6 +1570,19 @@ synthesizeCoroutineAccessorBody(AccessorDecl *accessor, ASTContext &ctx) {
? TargetImpl::Ordinary
: TargetImpl::Implementation);

// If this is a variable with an attached property wrapper, then
// the accessors need to yield the wrappedValue or projectedValue.
if (auto var = dyn_cast<VarDecl>(storage)) {
if (var->hasAttachedPropertyWrapper()) {
target = TargetImpl::Wrapper;
}

if (var->getOriginalWrappedProperty(
PropertyWrapperSynthesizedPropertyKind::StorageWrapper)) {
target = TargetImpl::WrapperStorage;
}
}

Copy link
Contributor

@roop roop Dec 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@theblixguy I don't understand why this is required.

Given a property like:

struct Foo {
  @Wrap var bar: Int
}

Before this PR, the synthesized modify in SIL looked like:

Foo.bar.modify:
  var tmp = call Foo.bar.getter(self)
  yield &tmp
  call Foo.bar.setter(tmp, &self)

where the Foo.text.getter calls Wrap.wrappedValue.getter, and Foo.text.setter calls Wrap.wrappvedValue.setter.

After this PR, it looks like:

Foo.bar.modify:
  var tmp = call Wrap.wrappedValue.getter(self._text)
  yield &tmp
  call Wrap.wrappedValue.setter(tmp)

These are equivalent, but I thought it was cleaner that Foo.bar.modify calls into Foo.bar.getter/setter instead of doing something special for property wrappers. Why is it being done the latter way?

SourceLoc loc = storage->getLoc();
SmallVector<ASTNode, 1> body;

Expand Down Expand Up @@ -1602,8 +1613,11 @@ synthesizeReadCoroutineBody(AccessorDecl *read, ASTContext &ctx) {
static std::pair<BraceStmt *, bool>
synthesizeModifyCoroutineBody(AccessorDecl *modify, ASTContext &ctx) {
#ifndef NDEBUG
auto impl = modify->getStorage()->getReadWriteImpl();
assert(impl != ReadWriteImplKind::Modify &&
auto storage = modify->getStorage();
auto impl = storage->getReadWriteImpl();
auto hasWrapper = isa<VarDecl>(storage) &&
cast<VarDecl>(storage)->hasAttachedPropertyWrapper();
assert((hasWrapper || impl != ReadWriteImplKind::Modify) &&
impl != ReadWriteImplKind::Immutable);
#endif
return synthesizeCoroutineAccessorBody(modify, ctx);
Expand Down Expand Up @@ -2249,9 +2263,15 @@ PropertyWrapperMutabilityRequest::evaluate(Evaluator &,
isProjectedValue = true;
}

if (var->getParsedAccessor(AccessorKind::Get))
// Make sure we don't ignore .swiftinterface files, because those will
// have the accessors printed
auto varSourceFile = var->getDeclContext()->getParentSourceFile();
auto isVarNotInInterfaceFile =
varSourceFile && varSourceFile->Kind != SourceFileKind::Interface;

if (var->getParsedAccessor(AccessorKind::Get) && isVarNotInInterfaceFile)
return None;
if (var->getParsedAccessor(AccessorKind::Set))
if (var->getParsedAccessor(AccessorKind::Set) && isVarNotInInterfaceFile)
return None;

// Figure out which member we're looking through.
Expand Down Expand Up @@ -2548,7 +2568,8 @@ static void finishPropertyWrapperImplInfo(VarDecl *var,
}

if (wrapperSetterIsUsable)
info = StorageImplInfo::getMutableComputed();
info = StorageImplInfo(ReadImplKind::Get, WriteImplKind::Set,
ReadWriteImplKind::Modify);
else
info = StorageImplInfo::getImmutableComputed();
}
Expand Down
2 changes: 2 additions & 0 deletions test/ModuleInterface/property_wrappers.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ public struct HasWrappers {
// CHECK: @TestResilient.Wrapper public var x: {{(Swift.)?}}Int {
// CHECK-NEXT: get
// CHECK-NEXT: set
// CHECK-NEXT: _modify
// CHECK-NEXT: }
@Wrapper public var x: Int

Expand All @@ -59,6 +60,7 @@ public struct HasWrappers {
// CHECK: @TestResilient.WrapperWithInitialValue @_projectedValueProperty($z) public var z: Swift.Bool {
// CHECK-NEXT: get
// CHECK-NEXT: set
// CHECK-NEXT: _modify
// CHECK-NEXT: }
@WrapperWithInitialValue(alternate: false) public var z
}
76 changes: 76 additions & 0 deletions test/SILGen/property_wrapper_coroutine.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
// RUN: %target-swift-emit-silgen %s | %FileCheck %s

@propertyWrapper
struct TestWrapper<ValueType> {
var wrappedValue: ValueType
}

struct State {
@TestWrapper var values: [String] = []
}

protocol StateProtocol {
@_borrowed var someValues: [String] { get set }
}

struct State1: StateProtocol {
@TestWrapper var someValues: [String] = []
}

var state = State()
state.values = Array(repeating: "", count: 20000)
state.values[1000] = "foo"

let state1 = State1()
_ = state1.someValues

// >> Check that the subscript assignment uses the _modify coroutine

// CHECK: {{%.*}} = begin_access [modify] [dynamic] {{%.*}} : $*State
// CHECK: [[REF_VALUES_MODIFY:%.*]] = function_ref @$s26property_wrapper_coroutine5StateV6valuesSaySSGvM : $@yield_once @convention(method) (@inout State) -> @yields @inout Array<String>
// CHECK: ([[RES1:%.*]], {{%.*}}) = begin_apply [[REF_VALUES_MODIFY]]({{%.*}}) : $@yield_once @convention(method) (@inout State) -> @yields @inout Array<String>
// CHECK: [[REF_ARRAY_SUBSCRIPT:%.*]] = function_ref @$sSayxSiciM : $@yield_once @convention(method) <τ_0_0> (Int, @inout Array<τ_0_0>) -> @yields @inout τ_0_0
// CHECK: ({{%.*}}, {{%.*}}) = begin_apply [[REF_ARRAY_SUBSCRIPT]]<String>({{%.*}}, [[RES1]]) : $@yield_once @convention(method) <τ_0_0> (Int, @inout Array<τ_0_0>) -> @yields @inout τ_0_0

// >> Check that the _modify coroutine is synthesized properly

// CHECK-LABEL: sil hidden [transparent] [ossa] @$s26property_wrapper_coroutine5StateV6valuesSaySSGvM : $@yield_once @convention(method) (@inout State) -> @yields @inout Array<String> {
// CHECK: bb0([[STATE:%.*]] : $*State):
// CHECK: debug_value_addr [[STATE]] : $*State, var, name "self", argno {{.*}}
// CHECK: [[BEGIN_ACCESS:%.*]] = begin_access [modify] [unknown] [[STATE]] : $*State
// CHECK: [[BACKING_ADDR:%.*]] = struct_element_addr [[BEGIN_ACCESS]] : $*State, #State._values
// CHECK: [[VALUE_ADDR:%.*]] = struct_element_addr [[BACKING_ADDR]] : $*TestWrapper<Array<String>>, #TestWrapper.wrappedValue
// CHECK: yield [[VALUE_ADDR]] : $*Array<String>, resume bb1, unwind bb2
//
// CHECK: bb1:
// CHECK: end_access [[BEGIN_ACCESS]] : $*State
// CHECK: [[RETURN:%.*]] = tuple ()
// CHECK: return [[RETURN]] : $()
//
// CHECK: bb2:
// CHECK: end_access [[BEGIN_ACCESS]] : $*State
// CHECK: unwind
// CHECK-END: }

// >> Check that the _read coroutine is synthesized properly

// CHECK-LABEL: sil shared [ossa] @$s26property_wrapper_coroutine6State1V10someValuesSaySSGvr : $@yield_once @convention(method) (@guaranteed State1) -> @yields @guaranteed Array<String> {
// CHECK: bb0([[STATE1:%.*]] : @guaranteed $State1):
// CHECK: debug_value [[SELF:%.*]] : $State1, let, name "self", argno {{.*}}
// CHECK: [[EXTRACT_VALUE:%.*]] = struct_extract %0 : $State1, #State1._someValues
// CHECK: [[COPY_VALUE:%.*]] = copy_value [[EXTRACT_VALUE]] : $TestWrapper<Array<String>>
// CHECK: [[BEGIN_BORROW:%.*]] = begin_borrow [[COPY_VALUE]] : $TestWrapper<Array<String>>
// CHECK: [[EXTRACT_WRAPPEDVALUE:%.*]] = struct_extract [[BEGIN_BORROW]] : $TestWrapper<Array<String>>, #TestWrapper.wrappedValue
// CHECK: yield [[EXTRACT_WRAPPEDVALUE]] : $Array<String>, resume bb1, unwind bb2
//
// CHECK: bb1:
// CHECK: end_borrow [[BEGIN_BORROW]] : $TestWrapper<Array<String>>
// CHECK: destroy_value [[COPY_VALUE]] : $TestWrapper<Array<String>>
// CHECK: [[RETURN:%.*]] = tuple ()
// CHECK: return [[RETURN]] : $()
//
// CHECK: bb2:
// CHECK: end_borrow [[BEGIN_BORROW]] : $TestWrapper<Array<String>>
// CHECK: destroy_value [[COPY_VALUE]] : $TestWrapper<Array<String>>
// CHECK: unwind
// CHECK-END: }
2 changes: 1 addition & 1 deletion test/decl/var/property_wrappers_synthesis.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ struct UseWrapper<T: DefaultInit> {

// CHECK: accessor_decl{{.*}}_modify_for=wrapped
// CHECK: yield_stmt
// CHECK: member_ref_expr{{.*}}UseWrapper.wrapped
// CHECK: member_ref_expr{{.*}}Wrapper.wrappedValue
@Wrapper
var wrapped = T()

Expand Down