You can obtain a val from a box field via recover

[chalcolith]

In the following code

class Foo
  let s: String box
  
  new create(s': String box) =>
    s = s'
  
  fun get_s(): String val =>
    recover val s end

actor Main
  new create(env: Env) =>
    let s = String
    s.append("world")
    let foo = Foo(s)
    env.out.print("hello " + foo.get_s())

The get_s() function should not compile, as recovering a val from a box is a leak in the type system.

[jemc]

This is the error message we should be getting here:

ponyc/src/libponyc/expr/reference.c

Lines 283 to 293 in 0f50e3c

	if(!sendable(type))
	{
	if(!sendable(l_type))
	{
	errorframe_t frame = NULL;
	ast_error_frame(&frame, ast, "can't access non-sendable field of "
	"non-sendable object inside of a recover expression");
	errorframe_report(&frame, opt->check.errors);
	return false;
	}
	}

[jasoncarr0]

This seems to be related to the sendability check for arrow types:

  fun get_s(): String val =>
    let f: this->(Foo box) = this // this works
    // let f: (Foo box) = this // this doesn't
    recover val f.s end

https://playground.ponylang.io/?gist=b6e61f17abf5e3f0b46f68e65ec2ddf6

[SeanTAllen]

@jasoncarr0 @jemc at sync today we merged a triggers release issue fix. I am prepping to do a release for that, however, if you think this would be fixed "soon-ish" like within a few days, I would hold the release to include this as well.

Should I hold or go ahead with the release?

[jemc]

@SeanTAllen - I say go ahead with the release.

[jasoncarr0]

I would say go ahead because I'm not sure how long it would take for a fix depending on the details of the issue.

[SeanTAllen]

@jasoncarr0 checking it to see if you are making progress on this.

[jasoncarr0]

I haven't been making progress / working on this much, I know that the issue is that the current code takes the lower bound (so that lb(this->box) turns into lb({ref,val,box}->box) turns into lb({box,val,box}) into val which counts as sendable for the recover block). But trying to fix this naively just ran into some errors about what is supported. It's probably something very simple

[SeanTAllen]

So the issue here is that we have

(-> thistype (nominal (id $0) (id String) x box x x))

which in viewpoint_lower hits this unfortunate block:

    case TK_THISTYPE:
      l_cap = TK_CAP_READ;
      break;

And whee! we are going to be sendable because in cap_view_lower we hit:

    case TK_CAP_READ:
    {
      switch(*right_cap)
      {
        case TK_ISO:
          *right_cap = TK_CAP_SEND;
          break;

        case TK_REF:
          *right_cap = TK_CAP_READ;
          break;

        case TK_BOX:
          *right_cap = TK_VAL;
          break;

        default: {}
      }
      break;
    }

where our box "becomes" a val.

@jemc does that sound correct to you? i'm not sure where to go to find the what is the correct thing to do (but I know this isn't correct).

[jemc]

@SeanTAllen - yes, that matches Jason's description of the problem, from above:

I know that the issue is that the current code takes the lower bound (so that lb(this->box) turns into lb({ref,val,box}->box) turns into lb({box,val,box}) into val

[SeanTAllen]

This error exists all the way back on 0.13.0 so this is a long standing bug.

[jasoncarr0]

Honestly getting the notifications and looking at this again, I think the only obviously correct solution is checking all instantiations and hence something like a for_all_instantiations function is necessary.

Possibly because sendability as a condition is invariant, not co nor contravariant.

The slightly nicer solution with infinite spare dev-hours is to replace all uses of sendability with a co-/contra-variant version. E.g. in recover blocks, we don't check sendability, we simply pick the best sendable approximation. But that doesn't quite work to replace the special casing on fields because iso->box is tag.

[SeanTAllen]

I need to check with @sylvanc to see if my fix that passes all tests including regressions for this is in fact sound and correct.

[SeanTAllen]

i was close, but, not quite there. i had the implementation "not quite right from a theory perspective". when i have time, i'll get the 3 regressions from this issue in place and open a PR to close then and then we can do a release.