Bug or intended? "thing?.property = something" will trigger didSet observer even if thing is nil, but ONLY IF thing is a struct type

Try this:

struct Thing {
    var property: Int = 0
}

var optionalThing: Thing? {
    didSet { print("didSet") }
}

optionalThing?.property = 1

It will print "didSet" even though nothing was set, obviously.

This does NOT happen if you change struct to class

It's a little annoying because I want to set properties on things if they're not nil, and log a change when it happens. This behavior leads to superfluous log entries, or requires me to type if optionalThing != nil { optionalThing!.property = 1 }

2 Likes

structs are value types. Changing part of a value means mutating the value itself – the reason why didSet is triggered.

class types, unlike structs, are reference types.

P.S. Changing it to a class won't hurt if you need that observer.

3 Likes

Afaics, the surprise is that there is no value that could be mutated, but didSet is triggered anyways - and I agree that this isn't obvious

It is however a bit strange that didSet is triggered optionalThing being nil. @Tino pointed this out while I was writing. Seems to be how the compiler works, since the dot notation isn't an operator. Perhaps @xwu knows why.

If somebody asked me to explain optional chaining, I'd say "check if part left of ?. is nil and go on, or abort (and possibly return nil)"
Afaik, this is true for all cases except this one - and if there isn't an explanation at least as simple as mine which takes into account that didSet is called, I'd say it is indeed something that should be fixed.

1 Like

@SubjugaterCephalopod I believe it makes sense to clarify this as well:

optionalThing is of type Thing? aka Optional<Thing>, which is an enum (Optional is a generic enum), not a struct.

Note that enums too are value types.

So, if we suppose optionalThing isn't nil, what really happens is that you are mutating property, hence mutating your struct instance, hence mutating the enum instance, a part of which the struct instance is, ultimately triggering didSet on the enum instance (optionalThing).

Yes of course, but, uh... optionalThing is nil. My understanding matches @Tino's: optionalThing fails the "is not nil" test, so how can the (unexecuted) property setting have any effect at all?

Yes, surely I understand your and @Tino's concern. It isn't logical to me either. More like didTryToSet {} rather that didSet {}.

Could the didSet code be written in such a way as to unintentionally crash if the optionalThing is nil? that behavior would make it a bug in my view.

C. Keith Ray

Just checked what is happening with a breakpoint. For value types, in this particular case the assembler instructions are the same and the observers are always triggered regardless of whether the variable is nil or not. If it is nil, well, it assigns nil again.

It's as if the program doesn't care, it accesses the memory of the property being mutated by calculating the position at which it has to be if exists, makes the necessary changes and then calls the observers.

By the way, in the example below the observers are also called despite nothing happening. Although of course this is a much less trivial case for the compiler.

So, it seems trying to mutate a value type also counts as a mutation and very likely this is intended.

struct A {
    var foo: Int {
        get { return 0 }
        set {}
    }
}

var a: A? = A() {
    didSet { print("tttt") }
}

a?.foo = 6

It might be worth noting that while the above program compiles with eg the default toolchain of Xcode 9.3 beta 4, it crashes recent versions of the compiler (for example dev snapshot 2018-02-25, 2018-03-13 and 2018-03-17, have only tested those three).

The following smaller program demonstrates the same issue (crashing recent versions of the compiler, but compiling and running, printing "hello", with eg default toolchain of Xcode 9.3 beta 4):

var a: Int? {
    didSet { print("hello") }
}
a? = 123

Filed SR-7220.

1 Like

This is very interesting. It's not just the nil; it's the whole wrapper.

I took a look at the SIL version to see if it would shed any additional light on the intent of the implementation here. My SIL-fu is pretty rudimentary, but it seems clear that the SIL copies the entire composite variable value onto the stack, then makes sure that all execution branches end with the value being copied back to the heap in toto. The didSet activation is just a side-effect of running the variable's setter to copy back the contents from the stack.

This put me in mind of the point you had made earlier about the variable in question being just an agglomeration of value types (an int within a struct within an enum), so I thought perhaps the compiler was just doing a not-very-optimized global setup and teardown that could accommodate whatever changes might happen to be made by the call chain as a whole.

So, I also looked at the SIL for this code, which adds an additional layer:

struct Thing {
    var intProperty: Int = 0 {
        didSet { print("intProperty of Thing set") } // Not called
    }
}

struct ThingHolder {
    var aThing: Thing? {
        didSet { print("aThing set") } // Called first
    }
}

var globalVariable = ThingHolder() {
    didSet { print("global variable set") } // Called second
}

globalVariable.aThing?.intProperty = 42

The surprising thing here (at least to me) is that not only does the SIL copy the contents of globalVariable to the stack, as in the previous example, but it also then makes a second copy of the Optional<Thing> portion of the struct on the stack.

After the call chain completes, the SIL calls the aThing setter to recopy the entire Optional<Thing> from the second stack copy back to the first. The didSet call for aThing is actually being run on a temporary copy of aThing that's held on the stack, but I guess that's fine since it's a value type and the code would never know the difference. Then, the SIL calls the setter for the global variable to copy the entire struct from the stack back to the heap.

So no, evidently the compiler is not recognizing that it's dealing with one big composite of value types and simply figuring that it can treat them all as one lump of data. It's repeating the same unpacking process at every layer of nesting. So in theory, it should be able to detect and omit the needless copy-back wherever the nil happens to be discovered within in the call chain.

It seems like a lot of extra work is being done, but perhaps the optimizer takes the actual code of the getters and setters into account and can omit some of the copying that appears to be going on in the SIL.

1 Like

Very interesting! I tested your program (and all the other examples in this thread) with a recent development snapshot and it turns out that all these examples crashes recent versions of the compiler. So something has changes since the default toolchain of Xcode 9.3 beta 4 (which is the last version I have access to that successfully compiles these programs).

(Including this again for completeness: SR-7220 )

The compiler crash looks similar for all examples:

Assertion failed: (value && "No value specified"), function forUnmanaged, file /Users/buildnode/jenkins/workspace/oss-swift-package-osx/swift/lib/SILGen/ManagedValue.h, line 91.
0  swift                    0x0000000108fcd2c8 llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 40
1  swift                    0x0000000108fcd9d6 SignalHandler(int) + 694
2  libsystem_platform.dylib 0x00007fff6e86df5a _sigtramp + 26
3  libsystem_platform.dylib 0x0000000000000009 _sigtramp + 2440634569
4  libsystem_c.dylib        0x00007fff6e698312 abort + 127
5  libsystem_c.dylib        0x00007fff6e660368 basename_r + 0
6  swift                    0x0000000105fc9997 swift::Lowering::LogicalPathComponent::writeback(swift::Lowering::SILGenFunction&, swift::SILLocation, swift::Lowering::ManagedValue, swift::Lowering::MaterializedLValue, bool) + 983
7  swift                    0x0000000105fd9a9b (anonymous namespace)::GetterSetterComponent::writeback(swift::Lowering::SILGenFunction&, swift::SILLocation, swift::Lowering::ManagedValue, swift::Lowering::MaterializedLValue, bool) + 219
8  swift                    0x0000000105fd6a26 (anonymous namespace)::LValueWritebackCleanup::emit(swift::Lowering::SILGenFunction&, swift::CleanupLocation) + 262
9  swift                    0x0000000105f27668 swift::Lowering::CleanupManager::emitCleanups(swift::DiverseStackBase::stable_iterator, swift::CleanupLocation, bool) + 296
10 swift                    0x0000000105f27a2b swift::Lowering::CleanupManager::emitBranchAndCleanups(swift::Lowering::JumpDest, swift::SILLocation, llvm::ArrayRef<swift::SILValue>) + 91
11 swift                    0x0000000105f34e0a swift::Lowering::SwitchCaseFullExpr::exitAndBranch(swift::SILLocation, llvm::ArrayRef<swift::SILValue>) + 314
12 swift                    0x0000000105fbe58d std::__1::__function::__func<swift::Lowering::SILGenFunction::emitBindOptional(swift::SILLocation, swift::Lowering::ManagedValue, unsigned int)::$_2, std::__1::allocator<swift::Lowering::SILGenFunction::emitBindOptional(swift::SILLocation, swift::Lowering::ManagedValue, unsigned int)::$_2>, void (swift::Lowering::ManagedValue, swift::Lowering::SwitchCaseFullExpr&&)>::operator()(swift::Lowering::ManagedValue&&, swift::Lowering::SwitchCaseFullExpr&&) + 61
13 swift                    0x0000000105f35864 swift::Lowering::SwitchEnumBuilder::emit() && + 1812
14 swift                    0x0000000105fa95d7 swift::Lowering::SILGenFunction::emitBindOptional(swift::SILLocation, swift::Lowering::ManagedValue, unsigned int) + 599
15 swift                    0x0000000105fd1a08 SILGenLValue::visitBindOptionalExpr(swift::BindOptionalExpr*, swift::AccessKind, swift::Lowering::LValueOptions) + 440
16 swift                    0x0000000105fcd283 swift::ASTVisitor<SILGenLValue, swift::Lowering::LValue, void, void, void, void, void, swift::AccessKind, swift::Lowering::LValueOptions>::visit(swift::Expr*, swift::AccessKind, swift::Lowering::LValueOptions) + 499
17 swift                    0x0000000105fcd67f SILGenLValue::visitRec(swift::Expr*, swift::AccessKind, swift::Lowering::LValueOptions, swift::Lowering::AbstractionPattern) + 223
18 swift                    0x0000000105fcfbea SILGenLValue::visitMemberRefExpr(swift::MemberRefExpr*, swift::AccessKind, swift::Lowering::LValueOptions) + 250
19 swift                    0x0000000105fcd25a swift::ASTVisitor<SILGenLValue, swift::Lowering::LValue, void, void, void, void, void, swift::AccessKind, swift::Lowering::LValueOptions>::visit(swift::Expr*, swift::AccessKind, swift::Lowering::LValueOptions) + 458
20 swift                    0x0000000105fccfce swift::Lowering::SILGenFunction::emitLValue(swift::Expr*, swift::AccessKind, swift::Lowering::LValueOptions) + 46
21 swift                    0x0000000105fbcf86 emitSimpleAssignment(swift::Lowering::SILGenFunction&, swift::SILLocation, swift::Expr*, swift::Expr*) + 1206
22 swift                    0x0000000105fad5ea swift::ASTVisitor<(anonymous namespace)::RValueEmitter, swift::Lowering::RValue, void, void, void, void, void, swift::Lowering::SGFContext>::visit(swift::Expr*, swift::Lowering::SGFContext) + 5146
23 swift                    0x0000000105fa1589 swift::Lowering::SILGenFunction::emitRValueAsSingleValue(swift::Expr*, swift::Lowering::SGFContext) + 57
24 swift                    0x0000000105f8231d swift::Lowering::SILGenFunction::emitOptionalSome(swift::SILLocation, swift::SILType, llvm::function_ref<swift::Lowering::ManagedValue (swift::Lowering::SILGenFunction&, swift::SILLocation, swift::Lowering::SGFContext)>, swift::Lowering::SGFContext) + 525
25 swift                    0x0000000105fb2039 swift::ASTVisitor<(anonymous namespace)::RValueEmitter, swift::Lowering::RValue, void, void, void, void, void, swift::Lowering::SGFContext>::visit(swift::Expr*, swift::Lowering::SGFContext) + 24169
26 swift                    0x0000000105fa1589 swift::Lowering::SILGenFunction::emitRValueAsSingleValue(swift::Expr*, swift::Lowering::SGFContext) + 57
27 swift                    0x0000000105fbaa4b void llvm::function_ref<void (llvm::SmallVectorImpl<swift::Lowering::ManagedValue>&, swift::Lowering::SGFContext)>::callback_fn<(anonymous namespace)::RValueEmitter::visitOptionalEvaluationExpr(swift::OptionalEvaluationExpr*, swift::Lowering::SGFContext)::$_14>(long, llvm::SmallVectorImpl<swift::Lowering::ManagedValue>&, swift::Lowering::SGFContext) + 123
28 swift                    0x0000000105fa9bca swift::Lowering::SILGenFunction::emitOptionalEvaluation(swift::SILLocation, swift::Type, llvm::SmallVectorImpl<swift::Lowering::ManagedValue>&, swift::Lowering::SGFContext, llvm::function_ref<void (llvm::SmallVectorImpl<swift::Lowering::ManagedValue>&, swift::Lowering::SGFContext)>) + 794
29 swift                    0x0000000105fae0f1 swift::ASTVisitor<(anonymous namespace)::RValueEmitter, swift::Lowering::RValue, void, void, void, void, void, swift::Lowering::SGFContext>::visit(swift::Expr*, swift::Lowering::SGFContext) + 7969
30 swift                    0x0000000105fa1b47 swift::Lowering::SILGenFunction::emitIgnoredExpr(swift::Expr*) + 1287
31 swift                    0x0000000105f3f254 swift::Lowering::SILGenModule::visitTopLevelCodeDecl(swift::TopLevelCodeDecl*) + 388
32 swift                    0x0000000105f3fa5b swift::Lowering::SILGenModule::emitSourceFile(swift::SourceFile*, unsigned int) + 827
33 swift                    0x0000000105f40810 swift::SILModule::constructSIL(swift::ModuleDecl*, swift::SILOptions&, swift::FileUnit*, llvm::Optional<unsigned int>, bool) + 352
34 swift                    0x0000000105f40e1f swift::performSILGeneration(swift::FileUnit&, swift::SILOptions&, llvm::Optional<unsigned int>) + 95
35 swift                    0x000000010582cae6 performCompile(swift::CompilerInstance&, swift::CompilerInvocation&, llvm::ArrayRef<char const*>, int&, swift::FrontendObserver*, swift::UnifiedStatsReporter*) + 8598
36 swift                    0x000000010582993e swift::performFrontend(llvm::ArrayRef<char const*>, char const*, void*, swift::FrontendObserver*) + 3310
37 swift                    0x00000001057e7a33 main + 2051
38 libdyld.dylib            0x00007fff6e5ec115 start + 1
Stack dump:
0.  Program arguments: /Library/Developer/Toolchains/swift-DEVELOPMENT-SNAPSHOT-2018-03-17-a.xctoolchain/usr/bin/swift -frontend -c -primary-file test.swift -target x86_64-apple-darwin17.4.0 -enable-objc-interop -color-diagnostics -module-name test -o /var/folders/50/br4kxvjd0t551h0fmtrzkwdw0000gn/T/test-280f25.o 
<unknown>:0: error: unable to execute command: Abort trap: 6
<unknown>:0: error: compile command failed due to signal 6 (use -v to see invocation)
2 Likes

Considering we have

struct Thing {
  var foo: Int
}
var thing: Thing? = nil {
  didSet {
    print(thing)
  }
}

Then if we consider that:

  • any assignment will fire the didSet (even when the value doesn't effectively change). i.e. thing = nil will fire the didSet.
  • optional chaining acts like map or flatMap depending on what is chained (i.e. on whether the chained expression returns an Optional or not).

Then we have that

thing?.foo = 1

is equivalent to

thing = thing.map({
  var newThing = $0
  newThing.foo = 1
  return newThing
})

which evaluates to thing = nil and fires the didSet as expected.

I still find it a bit surprising, as it's not completely clear whether ?. is really just map/flatMap in disguise or not, but it has an explanation.

That does appear to be the current behavior, but it seems like a rather strange and inefficient way of defining the chaining operation to me.

When I was looking at the SIL dumps mentioned above, I also checked a version of the code that did not include didSet clauses. This change causes the variables in question to not have a dedicated setter routine; if their values are modified, the SIL code just stores a new value into the original location.

Interestingly, in this configuration, the line

variable?.property = 1

does not copy back the variable's value from the stack. That part happens only when there is a setter for variable.

That doesn't really clarify the design intent, but it does suggest that the copy-back might be there solely and explicitly to invoke the didSet clause.

Aha, this pretty much explains everything.

So simply put, each time it climbs one step higher and then goes all the way down the nesting again.

@GarthSnyder @Jens Should I file another one for the observers being called? It could be not related to SR-7220.

I was hoping someone more familiar with the implementation would jump in and say, "Of course it has to call the didSet routine, because of situation XYZ..." and we could all just smack our foreheads. I'd tag some folks here, but I'm not sure who among the usual suspects has been closest to this in the past.

I think it's definitely worth filing a bug if this thread doesn't unearth an explanation. Seems like it's probably a separate issue from SR-7220.

1 Like

Observer invocation issue SR-7257

1 Like