Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto-recover constructor expressions #4124

Merged
merged 12 commits into from
Jul 5, 2022
55 changes: 54 additions & 1 deletion src/libponyc/expr/call.c
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,51 @@ static bool apply_default_arg(pass_opt_t* opt, ast_t* param, ast_t** argp)
return true;
}

static ast_t* method_receiver_type(ast_t* method);

bool check_auto_recover_newref(ast_t* ast)
{
while (ast != NULL && ast_id(ast) != TK_CALL)
ast = ast_child(ast);

if (ast == NULL)
return false;

AST_GET_CHILDREN(ast, newref, positional, named);
if (ast_id(newref) != TK_NEWREF)
return false;

// sometimes newrefs are nested?
ast_t* child = ast_child(newref);
if (child != NULL && ast_id(child) == TK_NEWREF)
newref = child;

ast_t* receiver_type = method_receiver_type(newref);

ast_t* arg = ast_child(positional);
while (arg != NULL)
{
ast_t* arg_type = ast_type(arg);
if (is_typecheck_error(arg_type))
return false;

ast_t* arg_type_aliased = alias(arg_type);
bool ok = safe_to_autorecover(receiver_type, arg_type_aliased, WRITE);
ast_free_unattached(arg_type_aliased);

if (!ok)
return false;

arg = ast_sibling(arg);
}

token_id tcap = ast_id(cap_fetch(receiver_type));
if (tcap == TK_REF || tcap == TK_BOX)
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nice to instead eagerly return false if the capability doesn't match, that way you don't have to check the arguments if the return capability is already wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


return false;
}

static bool check_arg_types(pass_opt_t* opt, ast_t* params, ast_t* positional,
bool partial)
{
Expand Down Expand Up @@ -235,8 +280,16 @@ static bool check_arg_types(pass_opt_t* opt, ast_t* params, ast_t* positional,
return false;
}

ast_t* wp_type = consume_type(p_type, TK_NONE, false);
errorframe_t info = NULL;
ast_t* wp_type;
if(check_auto_recover_newref(arg))
{
wp_type = unisolated(p_type);
Copy link
Contributor

@jasoncarr0 jasoncarr0 May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't quite the right behavior, although it might be on the right track. You should be upgrading the argument/constructor capability instead trying to downgrade the parameter.

That way for instance, a String val argument can be fulfilled by auto-recovery of String (which is a ref constructor). It would also be a bit clearer what's going on that way. Or for another example, the linked issue used array val

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I've fixed this now.

}
else
{
wp_type = consume_type(p_type, TK_NONE, false);
}

if(wp_type == NULL)
{
Expand Down
1 change: 1 addition & 0 deletions src/libponyc/expr/call.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
PONY_EXTERN_C_BEGIN

bool method_check_type_params(pass_opt_t* opt, ast_t** astp);
bool check_auto_recover_newref(ast_t* ast);

bool expr_call(pass_opt_t* opt, ast_t** astp);

Expand Down
12 changes: 11 additions & 1 deletion src/libponyc/expr/operator.c
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include "postfix.h"
#include "control.h"
#include "reference.h"
#include "call.h"
#include "../ast/astbuild.h"
#include "../ast/id.h"
#include "../ast/lexer.h"
Expand Down Expand Up @@ -373,7 +374,16 @@ bool expr_assign(pass_opt_t* opt, ast_t* ast)

default: {}
}
ast_t* wl_type = consume_type(fl_type, TK_NONE, false);

ast_t* wl_type;
if(check_auto_recover_newref(right))
{
wl_type = unisolated(fl_type);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment to above, the r_type should be upgraded instead, as with recover blocks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done as above.

else
{
wl_type = consume_type(fl_type, TK_NONE, false);
}

// Assignment is based on the alias of the right hand side.
errorframe_t info = NULL;
Expand Down
5 changes: 1 addition & 4 deletions src/libponyc/type/cap.c
Original file line number Diff line number Diff line change
Expand Up @@ -1012,15 +1012,13 @@ bool cap_immutable_or_opaque(token_id cap)
return false;
}



static ast_t* modified_cap_single(ast_t* type, cap_mutation* mutation)
{
ast_t* cap = cap_fetch(type);
ast_t* ephemeral = ast_sibling(cap);

token_id cap_token = ast_id(cap);
token_id eph_token = ast_id(cap);
token_id eph_token = ast_id(ephemeral);
ergl marked this conversation as resolved.
Show resolved Hide resolved

if (mutation(&cap_token, &eph_token))
{
Expand Down Expand Up @@ -1153,4 +1151,3 @@ ast_t* unisolated(ast_t* type)
{
return modified_cap(type, unisolated_mod);
}

16 changes: 16 additions & 0 deletions test/libponyc/recover.cc
Original file line number Diff line number Diff line change
Expand Up @@ -738,3 +738,19 @@ TEST_F(RecoverTest, CanWriteTrn_TrnAutoRecovery)

TEST_COMPILE(src);
}
TEST_F(RecoverTest, CanAutorecover_Newref)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there should be a blank line here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we said that we would add all tests that we can to runner tests in which case this should be a runner test as we are only checking that it compiles.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I've moved it.

{
const char* src =
"actor Main\n"
" new create(env: Env) =>\n"
" Bar.take_foo(Foo(88))\n"
" let bar: Foo iso = Foo(88)\n"
"class Foo\n"
" new create(v: U8) =>\n"
" None\n"
"primitive Bar\n"
" fun take_foo(foo: Foo iso) =>\n"
" None\n";

TEST_COMPILE(src);
}