Throwable accessors

Slava_Pestov · February 16, 2019, 2:49am

You're missing a catch (or an outer 'throws' function) in your example, so SILGen does not know where to jump if f.bar throws. This suggests the code in TypeCheckError is not working right yet.

I would be open to moving the lvalue checking back to Sema from SILGen, as long as the corresponding code in SILGen went away. We used to have this calculated in two places, with the SILGen version being authoritative, and the code in Sema only used in a couple of places, which I think was a bad design.

suyashsrijan · February 16, 2019, 4:21pm

Ah ok, I guess I need to tweak it a bit to detect outer functions. I have this same issue with setters as well:

extension String: Error {}

struct Foo {
  var bar: String {
    get throws {}
    set throws { 
      throw "Throwing from a setter"
    }
  }
}

var instance = Foo()
try instance.bar = "" // Assertion failed: (ThrowDest.isValid() && "calling emitThrow with invalid throw destination!")

Is it due to the same problem (no outer function/catch statement)?

This works and compiles fine:

extension String: Error {}

class Foo {
  var bar: String {
    get throws {
      throw "Throwing from a getter"
    }
  }
}

let _ = try Foo().bar

suyashsrijan · February 16, 2019, 7:32pm

So it seems like I have got it working for the most part - weird thing is that when I pass -emit-sil or -emit-ir to swiftc, I get the output code. However, when I pass -emit-executable, it crashes with that same SILGen error. When I just call swift <filename>, it compiles and runs and crashes as expected. Any ideas why this is happening?

Example:

extension String: Error {}

struct Foo {
  var bar: String {
    mutating get throws { 
      throw "Throwing from a getter"
    }
    mutating set throws {
      throw "Throwing from a setter"
    }
  }
}

func foo(f: inout Foo) throws {
  try f.bar = ""
}

var instance = Foo()
try foo(f: &instance)

When calling ./swift file_name: Fatal error: Error raised at top level: "Throwing from a setter"

When calling ./swiftc -emit-executable file_name: Assertion failed: (ThrowDest.isValid() && "calling emitThrow with invalid throw destination!")

When calling ./swiftc -frontend -emit-ir: OK

When calling ./swiftc -frontend -emit-bc file_name -o file_name.bc) and running using lli: lli -load=/Applications/Xcode-beta.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift/macosx/libswiftCore.dylib -load=/Applications/Xcode-beta.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift/macosx/libswiftSwiftOnoneSupport.dylib file_name.bc: Fatal error: Error raised at top level: "Throwing from a setter"

Interesting... seems like the crash either happens or not depending on how I invoke the compiler.

suyashsrijan · February 16, 2019, 8:51pm

Still not sure why its crashing when calling from swift -emit-executable but not swiftc -frontend -emit-sil. Looking at the SIL code:

try foo(f: &instance)

try_apply %9(%8) : $@convention(thin) (@inout Foo) -> @error Error, normal bb1, error bb2
...
bb2(%16 : $Error):
  end_access %8 : $*Foo
  %18 = builtin "errorInMain"(%16 : $Error) : $()
  unreachable

&

try f.bar = ""

try_apply %10(%8, %9) : $@convention(method) (@owned String, @inout Foo) -> @error Error, normal bb1, error bb2
...
bb2(%16 : $Error):
  end_access %9 : $*Foo
  throw %16 : $Error

So, the SIL looks fine to me and as I said above, it does compile and work. Still trying to work out why it would cause SILGen to crash when calling swift but not swiftc. Or maybe I am doing something wrong when calling the compiler? cc @Slava_Pestov @John_McCall

suyashsrijan · February 17, 2019, 11:21pm

I think I have figured this out - seems like it's happening due to a synthesised coroutine accessor. synthesizeCoroutineAccessorBody() synthesises a body for the modify accessor yielding an InOutExpr, however it creates an infinite loop in SILGen if the setter throws. For some reason, this doesn't happen when I call swiftc -frontend -emit-sil but it does happen when I call swift -emit-sil. I don't even see the modify accessor in the output SIL code, so I guess perhaps its not needed if the accessor does not have storage? We can then skip synthesising a body:

if (!accessor->getStorage()->hasStorage()) { return; }

This solves the crash.

DevAndArtist · February 18, 2019, 6:25am

A little off-topic: I would love to see a document or a very detailed blog post of your experience starting contribution to the Swift compiler, please include all the gotcha moments you had. Something from zero to hero like is always interesting to follow as I‘m kind of sitting in the same boat right now and reading a C++ book to catch up with that before diving into the compiler itself.

suyashsrijan · February 18, 2019, 6:35pm

Still needs a bit more work for type checking subscripts correctly, which I am currently working on (this one is a bit tricky, but I'm close). What else needs to be done? Everything else seems to be working fine (compiles, runs, throws the error as expected) according to my tests, but unsure if I am missing some scenarios. @John_McCall @Slava_Pestov

suyashsrijan · February 18, 2019, 6:58pm

Just to give an example of what works so far:

Computed properties with throwing getters and setters
Subscripts with throwing getter and setter

Example:

struct Foo {
  private var _array: [Int] = [0, 1, 2]
  
  enum FooError: Error { case accessor }
  
  subscript(x: Int) -> Int {
    get throws {
      if _array.indices.contains(x) {
        return _array[x]
      } else {
        throw FooError.accessor
      }
    }
    set {
      _array[x] = newValue
    }
  }
  
  var array: [Int] {
    get throws {
      if !_array.isEmpty {
        return _array
      } else {
        throw FooError.accessor
      }
    }
    set throws {
      if newValue.isEmpty {
        throw FooError.accessor
      } else {
        _array = newValue
      }
    }
  }
}

var instance = Foo()
let _ = try instance[1] // 1
let _ = try instance[3] // FooError.accessor
let _ = try? instance[3] // Ok
instance[0] = 3 // Ok

var anotherInstance = Foo()
let _ = try anotherInstance.array // [0, 1, 2]
try anotherInstance.array = [] // FooError.accessor
try anotherInstance.array = [1, 2, 3] // Ok
try? anotherInstance.array = [] // Ok

Both mutating and non-mutating variants work.

John_McCall · February 18, 2019, 7:24pm

The main thing you need to do for type-checking is to resurrect an old piece of code we had that stored an AccessKind on l-value expressions. It was removed here. That should make it straightforward to figure out what accessors are being used.

That's probably something you should go ahead and prepare a PR for instead of leaving on your feature branch.

DevAndArtist · February 18, 2019, 7:27pm

Can you share a link to the WIP PR please? Since I haven't seen protocol related examples in this thread, I would like to know if in your tests you already created them.

protocol P {
  subscript(a: Int) -> Int { get throws }
  subscript(b: Int) -> Int { get throws set }
  subscript(c: Int) -> Int { get set throws }
  var a: Int { get throws }
  var b: Int { get throws set }
  var c: Int { get set throws }
}

Can a subscript and therefore get/set actually rethrow if it's argument was a throwing function?

Example:

struct S {
  subscript(x: () throws -> Int) -> String {
    get rethrows {
      return "\(try x())"
    }
  }
}

DevAndArtist · February 18, 2019, 7:33pm

@John_McCall would it be a requirement for this patch to make internal _read and _modify (or how they were called) also throwable or can this be easily a follow-up patch?

Not trying to make anything harder for @suyashsrijan, just asking for clarification, since I'm tracking this patch.

John_McCall · February 18, 2019, 7:36pm

The feature is not in any way complete without _read and _modify support. We automatically create _modify accessors for non-final storage in classes as well as storage that satisfies a mutable protocol requirement.

suyashsrijan · February 18, 2019, 7:37pm

Protocols with throwing accessors work although there's an issue where a throwing getter can fulfil the requirement of a non-throwing getter (as defined in protocol). rethrows doesn't get parsed, but unsure if it's something we want to add right now.

anandabits · February 18, 2019, 7:41pm

This sounds pretty bad. It will be fixed before this lands on master (assuming it is accepted), right?

suyashsrijan · February 18, 2019, 7:42pm

Yes, at the moment, I want to wrap up the type checking bit before moving on to other areas.

anandabits · February 18, 2019, 7:43pm

Great!

DevAndArtist · February 18, 2019, 7:43pm

In the sense of this post (which is not available in Swift right now and is not even decided yet) I would expect that any get/set (read/modify) throws all the time. If omitted then it throws Never and therefore works like in the current Swift version.

John_McCall · February 18, 2019, 7:45pm

I would not worry about rethrows. It's useful with subscripts that take closures, especially autoclosures, but it is definitely not a minimum requirement.

suyashsrijan · February 19, 2019, 4:04pm

Thanks @John_McCall, I have got the type checking working now for subscripts I am using a custom ASTVisitor approach and it works fine. I have a PR ready that reverts the commit you mentioned. Do you think it's worth reverting or should I stick to my approach?

John_McCall · February 19, 2019, 4:11pm

Are you actually handling inout arguments and potentially-mutating base accesses correctly? It’s certainly possible to do these things with a visitor, but my concern is that you’re going to be “approximately correct”.