Throwable accessors

Development Compiler
,
1

I have spent a bit of time looking into this based on @John_McCall suggestion and encouragement here.

I think the first thing is to start with the parser and update parseGetSet() to consume a throws token and then update createAccessorFunc() signature to accept a throws boolean so when creating an AccessorDecl, we can pass it forward.

I have started working on this feature, but I am wondering how much code can be reused since AccessorDecl is a subclass of FuncDecl which supports throws and what other (specific) areas need to be updated to support the new functionality? cc @John_McCall

4 Likes
2

I think modifying the parser is a fine place to start.

The biggest issue at this stage is probably going to be dealing with all the places that synthesize accessors and making a reasonable decision about whether the synthesized accessor should throw. In most places I hope it'll be obvious how to make that decision, but there might be some nastier exceptions.

1 Like
3

Thanks John, I have got the parser bit working now. At the moment, this parses:

class Foo {

  enum FooError: Error {
    case bar
  }

  var baz: String {
    get throws {
      throw FooError.bar
    }
  }
}

If I try using it:

let foo = Foo()
let hello = foo.baz

then it works! I get a fatal error: Fatal error: Error raised at top level: test.Foo.FooError.bar

Sema doesn't quite detect it when I add a try (it complains no calls to throwing functions occur within 'try' expression) but still leads to a crash as expected. try? works though & it doesn't crash.

1 Like
4

TypeCheckError is where you're going to want to go if you want the type checker to understand that an accessor can throw. Right now it only checks apply expressions if the callee throws.

5

Setters don't work at the moment, need some tweaking. Currently crashes in SIL: (ThrowDest.isValid() && "calling emitThrow with invalid throw destination!")

6

Yeah, I'll tweak that in a moment!

7

It would be great if at some point you could also look into updating the libSyntax part, e.g. the syntax definition of accessors :pray:

2 Likes
8

Will do :slight_smile:

9

The TypeCheckError stuff might be somewhat of a challenge — we used to mark how an l-value expression was being accessed in the AST, but @Slava_Pestov ripped that out, so now you'll have to propagate that information down to the DeclRefExpr etc. so that you can know which accessors are being used.

10

I think the reason why it's not getting detected for try is because the type of the property (VarDecl) is type='String' interface type='String' whereas the type for the accessor (get) is interface type='(Foo) -> () throws -> String', so in TypeCheckError, when checkTry tries walking the subexpression, it encounters a MemberRefExpr whose type is String and thus produces the no calls to throwing functions occur within 'try' expression warning.

So perhaps we also need to update the type of the property to () throws -> String(). Is that the correct approach? or should we teach ErrorHandlingWalker how to look for accessors as well? @John_McCall

11

It would make more sense (to me) to teach it to walk accessors because new expressions in the future, like await or unsafe, can take advantage of this for async or unsafe accessors.

1 Like
12

Yes, it seems like a reasonable approach. Still trying to understand how it works, because ErrorHandlingWalker is a simple class and doesn't seem to be doing much.

13

Right now, the walker assumes that expressions like DeclRefExpr and MemberRefExpr don't themselves throw because accessors never throw. That needs to change. Like I said above, this will be somewhat difficult because you can't tell from looking at a DeclRefExpr which accessors are going to be used.

14

@John_McCall Hmm, in the example I gave above, the try expression would look something like:

(try_expr ...)
  (member_ref_expr ...)
    (declref_expr ...)

So, a check like this would work:

// in checkTry()
auto throwingAccessor = false;
if (auto MRE = dyn_cast<MemberRefExpr>(E->getValueProvidingExpr())) {
	if (auto VD = dyn_cast_or_null<VarDecl>(MRE->getDecl().getDecl())) {
		if (VD->getAccessor(AccessorKind::Get)->hasThrows()) {
			throwingAccessor = true;
			Flags.set(ContextFlags::HasTryThrowSite);
		}
	}
}
		
if (!throwingAccessor) {
	E->getSubExpr()->walk(*this);
}

for

let bar = try foo.throwingProperty

This is just a simple check, but perhaps this could be expanded to check for DeclRefExpr and MemberRefExpr both and check for getters and setters as well. Maybe this could be extracted into a separate "walker" too.

15

But yes, as you said, it's tricky to determine which accessors are going to be used!

16

That's the right basic idea, except that you should base the decision on which accessor is actually being used, which is context-dependent.

2 Likes
17

@John_McCall Yes, that works for getters. For setters, we will have something like this:

(try_expr ...)
  (assign_expr ...)
    (member_ref_expr ...)
      (declref_expr ...)

so maybe check for an assign expression? That could be used to distinguish between a getter and a setter.

18

So,

auto throwingAccessor = false;
auto setter = dyn_cast<AssignExpr>(E->getValueProvidingExpr());
auto expr = setter ? setter->getDest() : E->getValueProvidingExpr();
if (auto MRE = dyn_cast<MemberRefExpr>(expr)) {
	if (auto VD = dyn_cast_or_null<VarDecl>(MRE->getDecl().getDecl())) {
		if (VD->getAccessor(setter ? AccessorKind::Set : AccessorKind::Get)->hasThrows()) {
			throwingAccessor = true;
			Flags.set(ContextFlags::HasTryThrowSite);
		}
	}
}
		
if (!throwingAccessor) {
    E->getSubExpr()->walk(*this);
}

...
19

That's more like it, but sadly it is much more complicated than that because of inout arguments, mutating methods, mutating accessors, and access semantics.

20

@John_McCall How should I proceed with that? mutating getter has the same issue as setters - crash in SILGen: Assertion failed: (ThrowDest.isValid() && "calling emitThrow with invalid throw destination!"). I have extremely limited experience with SIL so unsure why this is happening.

extension String: Error {}

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

func foo(f: inout Foo) {
  let _ = f.bar
}

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