"with blocks" with no "else" clause are desugared incorrectly

[ergl]

The tutorial mentions that the return type of a with block is "the value of the last expression in the block, or of the last expression in the else block if there is one and an error occurred."

However, the above is incorrect in cases where the body of a with block doesn't raise an error. In that case, the compiler will generate a try block that returns None in the else clause, causing the return type to always be a union type.

This makes with blocks impossible to use without error checking in cases where one is only interested in the auto-disposing behaviour. Consider the following code:

class Foo
  new ref create() => None
  fun apply(): String => ""
  fun dispose() => None

actor Main
  new create(env: Env) =>
    let s: String = with f = Foo do
      f.apply()
    else
      ""
    end

The above fails to compile with "try expression never results in an error", informing us that we don't need an else clause (although the message is confusing, since the user never used a try expression in the code).

If we change the above to remove the else clause:

class Foo
  new ref create() => None
  fun apply(): String => ""
  fun dispose() => None

actor Main
  new create(env: Env) =>
    let s: String = with f = Foo do
      f.apply()
    end

Now we're hit with:

private/tmp/with_test/main.pony:8:19: right side must be a subtype of left side
    let s: String = with f = Foo do
                  ^
    Info:
    /Users/ryan/.local/share/ponyup/ponyc-release-0.41.0-x86_64-darwin/packages/builtin/none.pony:1:1: None val^ is not a subtype of String val^
    primitive None is Stringable
    ^
    /Users/ryan/.local/share/ponyup/ponyc-release-0.41.0-x86_64-darwin/packages/builtin/none.pony:1:1: not every element of (String val | None val^) is a subtype of String val^
    primitive None is Stringable
    ^

The None type above comes from the desugared try block, as we can see from the AST:

(seq
    (= (let (id $1$0) x) (seq (reference (id Foo))))
    (try
        (seq:scope (= (let (id f) x) (reference (id $1$0))) (seq (call (. (reference (id f)) (id apply)) x x x)))
        (seq:scope (= (let (id f) x) (reference (id $1$0))) (seq (reference (id None))))
        (seq:scope (= (let (id f) x) (reference (id $1$0))) (call (. (reference (id f)) (id dispose)) x x x))
    )
)

[ergl]

I understand it might be a bit hard to fix the above given how the desugaring takes place.

One solution would be to modify the else clause so that it returns the same value as the try, perhaps by duplicating the AST and generating the same body for both clauses:

class Foo
  new ref create() => None
  fun apply(): String => ""
  fun dispose() => None

actor Main
  new create(env: Env) =>
    let s: String =
      // A new scope is created here
      let f = Foo
      // TK_TRY_NO_CHECK is used to avoid "try expression never results in an error"
      try f.apply() else f.apply() then f.dispose() end

[jemc]

I think this is duplicate of: #1571

[jasoncarr0]

One clean way to fix this here is to desugar into try-then (without an else, unless one was provided). This avoids having to add another unique concept to the compiler.

Secondarily, the try-then block typechecking should be fixed so that it does not result in a union with None if the try block cannot error (i.e. if an else block is not necessary). It's not clear how easy this would be in the compiler

[ergl]

@jemc Thanks for the duplicate check, I tried searching for an open issue but didn't find that one. Do you want to close either one as a dup?

[jemc]

I don't mind closing the older one. Done.

Just note for anyone reading along that the other ticket has a reference to a sync call in 2017 where this was discussed. In case anyone wants to go read that comment and/or listen to the recording from that sync call.

[SeanTAllen]

I just encountered this one. Boy is it annoying. Would be nice to see this one fixed.

[SeanTAllen]

So

One clean way to fix this here is to desugar into try-then (without an else, unless one was provided). This avoids having to add another unique concept to the compiler.

This doesnt work because everything else in the compiler expects there to be try/else/then and if there was no else for it be a None which replaced a TK_NONE in sugar_try.

actor Main
  new create(env: Env) =>
    let s: String = 
      try
        "foo"
      then
        ""
      end

said issue also causes the above code to not compile.

It appears from my quick attempt to change sugar_try that a nice chunk of code is expecting there to always be a try/else/then as the fully desugared form.

[SeanTAllen]

I have a simple fix for

class Foo
  new ref create() => None
  fun apply(): String => ""
  fun dispose() => None

actor Main
  new create(env: Env) =>
    let s: String = with f = Foo do
      f.apply()
    else
      ""
    end

Ill be opening a PR for that and then looking at making with more usable in general.

[SeanTAllen]

I don't see a good, not hack way of fixing with as sugar over try.

I discussed with Joe and we came up with a new construct that isn't sugar that with would use that would also get rid of the else and require you to use try yourself in your with if you have partial functions but otherwise, it would "intercept" errors and run the disposal before propagating the error up.

I am still trying to figure out if there is an in-between on this that keeps the existing while/else (which is kind of a hack to support the try sugar) because it wouldn't need to go through an RFC, however, the amount of mental energy I have put into that over the last 3 weeks is probably more than if I had added the new constructs and made with a first class entity.

[SeanTAllen]

so I'm thinking something along the lines of removing the sugaring of

TK_WITH and making TK_WITH a first class item that is maintained as a TK_WITH throughout the program compilation.

I had discussed with @jemc a more general mechanism, but I'd rather start with a specific mechanism for now and go from there, it could be generalized later.

Currently the parser has:

DEF(with);
  PRINT_INLINE();
  TOKEN(NULL, TK_WITH);
  ANNOTATE(annotations);
  RULE("with expression", withexpr);
  SKIP(NULL, TK_DO);
  RULE("with body", rawseq);
  IF(TK_ELSE, RULE("else clause", annotatedrawseq));
  TERMINATE("with expression", TK_END);
  DONE();

And we'd have:

DEF(with);
  PRINT_INLINE();
  TOKEN(NULL, TK_WITH);
  ANNOTATE(annotations);
  RULE("with expression", withexpr);
  SKIP(NULL, TK_DO);
  RULE("with body", rawseq);
  TERMINATE("with expression", TK_END);
  DONE();

Things we might need to modify (for some definition of modify):

• gen_error / ast_try_clause
• frame_push (and frame needs a with_expr much like frame->try_expr)
• gen_then_clauses (or at least how it works but applied to us)
• gen_with that is somewhat like gen_try
• parser definition
• need to add to treecheckdef.h
• is_expr_constructor
• is_result_needed
• is_method_result need to add TK_DISPOSING_BLOCK and remove the stray useless TK_WITH.
• find_antecedent_type should need handling at the same location as TK_TRY et al
• i dont think we need much/anything to match what expr_try is doing
• i dont think we need to do anything in refer_seq, but we might.
• i dont think that anything like refer_try is needed
• sugar_with
• reachable_expr
• [ x] sugar.cc

To figure out:

currently, in sugar.c build_dispose, we create the dispose_clause we need for disposing of our items. we then set as the then clause on the try we are making. I don't know from how the parser type is set up if we can do this and not have the clause be something that the user can provide.

how do we add this "not user settable clause"?

note to self:

i don't think we care about "jump away" as we do the same thing no matter what.

[SeanTAllen]

-- learning underway, this might not be possible --

Ok, so as I learn more. I can't keep TK_WITH past the sugar phase. I'll need to desugar to something else.

That something else is TK_DISPOSING_BLOCK

The desugared will look like:

SEQ
  (= (let (id $0) x) (seq (reference (id Disposable))))
  TK_DISPOSING_BLOCK
    SEQ
      (= (let (id a) x) (reference (id $0)))
      BODY HERE
    SEQ
       (= (let (id a) x) (reference (id $0)))
       (call (. (reference (id a)) (id dispose)) x x x)

[SeanTAllen]

Ok, I'm rolling now. Look out @ergl, before too long you'll have usable with blocks.

[SeanTAllen]

If anyone is curious, the work is on the branch with-as-first-class

[SeanTAllen]

My list of things to do/look at now that I'm rolling:

• gen_error / ast_try_clause
• frame_push (and frame needs a with_expr much like frame->try_expr)
• gen_then_clauses (or at least how it works but applied to us) (need to handle return, and continue + break as we might be in a loop
• gen_disposing_block that is somewhat like gen_try
• parser definition
• need to add to treecheckdef.h
• is_expr_constructor
• is_result_needed
• is_method_result need to add TK_DISPOSING_BLOCK and remove the stray useless TK_WITH.
• find_antecedent_type should need handling at the same location as TK_TRY et al
• expr_try
• refer_seq
• refer_disposable_block
• sugar_with
• reachable_expr
• sugar.cc
• figure out how to "rethrow" errors.
• rename ast_try_clause to be more generic

[ergl]

@SeanTAllen I'm excited, looking forward to having this done!

[SeanTAllen]

Ugh, I ran into a problem with gen_error and the assumption that error is handled by a try/else/then.

This is "workaround-able", but still... requires a bit of a think to get right and requires a bit of restructuring of existing logic.

[SeanTAllen]

continue, break needed to support being in a loop.

[SeanTAllen]

@ergl dun dun dun...

#4024

[SeanTAllen]

@ergl enjoy!