From 527864dd4e73d34df7d85cf1bb5e71ac5bafc020 Mon Sep 17 00:00:00 2001 From: stefandd Date: Wed, 27 Jul 2022 15:35:29 +0200 Subject: [PATCH 1/7] Fix for crash involving the use of private types used as default values This attempts to fix issues #4130 and #4153. The issue was when a private type in another package was used as a default value in a method call. Fix to #4130 Since it has been decided to treat this as a bug instead of a missing error, this PR implements the fix suggested by @ergl, namely using `lookup_try()` instead of lookup() in call.c's `check_partial_function_call()` since it allows to permit private types. Fix to #4153 This is also a simply fix to lookup.c that prevents a potential segfault by a dereferencing of `opt` (`typecheck_t* t = &opt->check;`) *before* `opt != NULL` was checked. As pointed out by @SeanTAllen, opt should not be NULL to begin with when lookup_nominal is called, and instead, an assert should be added and the NULL checks in that function removed. --- src/libponyc/type/lookup.c | 43 ++++++++++++++++++-------------------- src/libponyc/verify/call.c | 4 ++-- 2 files changed, 22 insertions(+), 25 deletions(-) diff --git a/src/libponyc/type/lookup.c b/src/libponyc/type/lookup.c index f0b75bda08..03ad5c4190 100644 --- a/src/libponyc/type/lookup.c +++ b/src/libponyc/type/lookup.c @@ -57,14 +57,14 @@ static deferred_reification_t* lookup_nominal(pass_opt_t* opt, ast_t* from, ast_t* orig, ast_t* type, const char* name, bool errors, bool allow_private) { pony_assert(ast_id(type) == TK_NOMINAL); + pony_assert(opt != NULL); typecheck_t* t = &opt->check; ast_t* def = (ast_t*)ast_data(type); AST_GET_CHILDREN(def, type_id, typeparams); const char* type_name = ast_name(type_id); - if(is_name_private(type_name) && (from != NULL) && (opt != NULL) - && !allow_private) + if(is_name_private(type_name) && (from != NULL) && !allow_private) { if(ast_nearest(def, TK_PACKAGE) != t->frame->package) { @@ -94,34 +94,31 @@ static deferred_reification_t* lookup_nominal(pass_opt_t* opt, ast_t* from, case TK_FUN: { // Typecheck default args immediately. - if(opt != NULL) + AST_GET_CHILDREN(find, cap, id, typeparams, params); + ast_t* param = ast_child(params); + + while(param != NULL) { - AST_GET_CHILDREN(find, cap, id, typeparams, params); - ast_t* param = ast_child(params); + AST_GET_CHILDREN(param, name, type, def_arg); - while(param != NULL) + if((ast_id(def_arg) != TK_NONE) && (ast_type(def_arg) == NULL)) { - AST_GET_CHILDREN(param, name, type, def_arg); - - if((ast_id(def_arg) != TK_NONE) && (ast_type(def_arg) == NULL)) - { - ast_t* child = ast_child(def_arg); + ast_t* child = ast_child(def_arg); - if(ast_id(child) == TK_CALL) - ast_settype(child, ast_from(child, TK_INFERTYPE)); + if(ast_id(child) == TK_CALL) + ast_settype(child, ast_from(child, TK_INFERTYPE)); - if(ast_visit_scope(¶m, pass_pre_expr, pass_expr, opt, - PASS_EXPR) != AST_OK) - return NULL; + if(ast_visit_scope(¶m, pass_pre_expr, pass_expr, opt, + PASS_EXPR) != AST_OK) + return NULL; - def_arg = ast_childidx(param, 2); + def_arg = ast_childidx(param, 2); - if(!coerce_literals(&def_arg, type, opt)) - return NULL; - } - - param = ast_sibling(param); + if(!coerce_literals(&def_arg, type, opt)) + return NULL; } + + param = ast_sibling(param); } break; } @@ -140,7 +137,7 @@ static deferred_reification_t* lookup_nominal(pass_opt_t* opt, ast_t* from, return NULL; } - if(is_name_private(name) && (from != NULL) && (opt != NULL) && !allow_private) + if(is_name_private(name) && (from != NULL) && !allow_private) { switch(ast_id(find)) { diff --git a/src/libponyc/verify/call.c b/src/libponyc/verify/call.c index 814d60e25d..c820819f9d 100644 --- a/src/libponyc/verify/call.c +++ b/src/libponyc/verify/call.c @@ -28,8 +28,8 @@ static bool check_partial_function_call(pass_opt_t* opt, ast_t* ast) ast_id(call_error) == TK_NONE || ast_id(call_error) == TK_DONTCARE); // Look up the original method definition for this method call. - deferred_reification_t* method_def = lookup(opt, receiver, ast_type(receiver), - ast_name(method)); + deferred_reification_t* method_def = lookup_try(opt, receiver, ast_type(receiver), + ast_name(method), true); // allow private types ast_t* method_ast = method_def->ast; deferred_reify_free(method_def); From 0eae6eff89903a1dc7f7404a704d822ab66c5824 Mon Sep 17 00:00:00 2001 From: stefandd Date: Wed, 27 Jul 2022 15:59:53 +0200 Subject: [PATCH 2/7] Revert "Fix for crash involving the use of private types used as default values" This reverts commit 527864dd4e73d34df7d85cf1bb5e71ac5bafc020. --- src/libponyc/type/lookup.c | 43 ++++++++++++++++++++------------------ src/libponyc/verify/call.c | 4 ++-- 2 files changed, 25 insertions(+), 22 deletions(-) diff --git a/src/libponyc/type/lookup.c b/src/libponyc/type/lookup.c index 03ad5c4190..f0b75bda08 100644 --- a/src/libponyc/type/lookup.c +++ b/src/libponyc/type/lookup.c @@ -57,14 +57,14 @@ static deferred_reification_t* lookup_nominal(pass_opt_t* opt, ast_t* from, ast_t* orig, ast_t* type, const char* name, bool errors, bool allow_private) { pony_assert(ast_id(type) == TK_NOMINAL); - pony_assert(opt != NULL); typecheck_t* t = &opt->check; ast_t* def = (ast_t*)ast_data(type); AST_GET_CHILDREN(def, type_id, typeparams); const char* type_name = ast_name(type_id); - if(is_name_private(type_name) && (from != NULL) && !allow_private) + if(is_name_private(type_name) && (from != NULL) && (opt != NULL) + && !allow_private) { if(ast_nearest(def, TK_PACKAGE) != t->frame->package) { @@ -94,31 +94,34 @@ static deferred_reification_t* lookup_nominal(pass_opt_t* opt, ast_t* from, case TK_FUN: { // Typecheck default args immediately. - AST_GET_CHILDREN(find, cap, id, typeparams, params); - ast_t* param = ast_child(params); - - while(param != NULL) + if(opt != NULL) { - AST_GET_CHILDREN(param, name, type, def_arg); + AST_GET_CHILDREN(find, cap, id, typeparams, params); + ast_t* param = ast_child(params); - if((ast_id(def_arg) != TK_NONE) && (ast_type(def_arg) == NULL)) + while(param != NULL) { - ast_t* child = ast_child(def_arg); + AST_GET_CHILDREN(param, name, type, def_arg); - if(ast_id(child) == TK_CALL) - ast_settype(child, ast_from(child, TK_INFERTYPE)); + if((ast_id(def_arg) != TK_NONE) && (ast_type(def_arg) == NULL)) + { + ast_t* child = ast_child(def_arg); - if(ast_visit_scope(¶m, pass_pre_expr, pass_expr, opt, - PASS_EXPR) != AST_OK) - return NULL; + if(ast_id(child) == TK_CALL) + ast_settype(child, ast_from(child, TK_INFERTYPE)); - def_arg = ast_childidx(param, 2); + if(ast_visit_scope(¶m, pass_pre_expr, pass_expr, opt, + PASS_EXPR) != AST_OK) + return NULL; - if(!coerce_literals(&def_arg, type, opt)) - return NULL; - } + def_arg = ast_childidx(param, 2); - param = ast_sibling(param); + if(!coerce_literals(&def_arg, type, opt)) + return NULL; + } + + param = ast_sibling(param); + } } break; } @@ -137,7 +140,7 @@ static deferred_reification_t* lookup_nominal(pass_opt_t* opt, ast_t* from, return NULL; } - if(is_name_private(name) && (from != NULL) && !allow_private) + if(is_name_private(name) && (from != NULL) && (opt != NULL) && !allow_private) { switch(ast_id(find)) { diff --git a/src/libponyc/verify/call.c b/src/libponyc/verify/call.c index c820819f9d..814d60e25d 100644 --- a/src/libponyc/verify/call.c +++ b/src/libponyc/verify/call.c @@ -28,8 +28,8 @@ static bool check_partial_function_call(pass_opt_t* opt, ast_t* ast) ast_id(call_error) == TK_NONE || ast_id(call_error) == TK_DONTCARE); // Look up the original method definition for this method call. - deferred_reification_t* method_def = lookup_try(opt, receiver, ast_type(receiver), - ast_name(method), true); // allow private types + deferred_reification_t* method_def = lookup(opt, receiver, ast_type(receiver), + ast_name(method)); ast_t* method_ast = method_def->ast; deferred_reify_free(method_def); From 7c18b7f49b85c114a923980443fe7c5f3bbb5c34 Mon Sep 17 00:00:00 2001 From: stefandd Date: Wed, 27 Jul 2022 16:06:46 +0200 Subject: [PATCH 3/7] Fix for crash involving the use of private types used as default values This attempts to fix #4130 This crash stems from the use of a private type as defined in another package when it was used as a default value in a method call. Since it has been decided to treat this as a bug instead of a missing error, this PR implements the fix suggested by @ergl, namely using `lookup_try()` instead of lookup() in call.c's `check_partial_function_call()` since the former can be configured to permit private types. Fix to #4153 This is a simply change to `lookup_nominal()` in lookup.c that prevents a potential segfault by a dereferencing of `opt` (`typecheck_t* t = &opt->check;`) *before* `opt != NULL` was checked. As pointed out by @SeanTAllen, opt should not be NULL to begin with when `lookup_nominal()` is called, and instead, an assert should be added and the NULL checks in that function removed. With this PR, the two examples below that crashed the compiler now both compile: Original example: ```pony // inside the "useful" package primitive _PrivateDefault actor Useful[A: Any val] fun tag config(value: (A | _PrivateDefault) = _PrivateDefault): Useful[A] => this // inside "main" use "useful" primitive This primitive That type Stuff is (This | That) actor Main new create(env: Env) => let u = Useful[Stuff].config() ``` Minimal example: ```pony // In the "lib" pacakge primitive _Private primitive Public fun apply[T](v: (T | _Private) = _Private): None => None // In main use lib = "lib" actor Main new create(env: Env) => let p = lib.Public.apply[U8]() env.out.print(p.string()) ``` --- src/libponyc/verify/call.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libponyc/verify/call.c b/src/libponyc/verify/call.c index 814d60e25d..c820819f9d 100644 --- a/src/libponyc/verify/call.c +++ b/src/libponyc/verify/call.c @@ -28,8 +28,8 @@ static bool check_partial_function_call(pass_opt_t* opt, ast_t* ast) ast_id(call_error) == TK_NONE || ast_id(call_error) == TK_DONTCARE); // Look up the original method definition for this method call. - deferred_reification_t* method_def = lookup(opt, receiver, ast_type(receiver), - ast_name(method)); + deferred_reification_t* method_def = lookup_try(opt, receiver, ast_type(receiver), + ast_name(method), true); // allow private types ast_t* method_ast = method_def->ast; deferred_reify_free(method_def); From 6391765e1fc18f2caded55015a45be70abfbd574 Mon Sep 17 00:00:00 2001 From: stefandd Date: Wed, 27 Jul 2022 18:32:08 +0200 Subject: [PATCH 4/7] Adding test runner --- .../lib/lib.pony | 4 ++++ .../main.pony | 6 ++++++ 2 files changed, 10 insertions(+) create mode 100644 test/libponyc-run/private-type-as-default-argument-in-public-function/lib/lib.pony create mode 100644 test/libponyc-run/private-type-as-default-argument-in-public-function/main.pony diff --git a/test/libponyc-run/private-type-as-default-argument-in-public-function/lib/lib.pony b/test/libponyc-run/private-type-as-default-argument-in-public-function/lib/lib.pony new file mode 100644 index 0000000000..4bcad15e24 --- /dev/null +++ b/test/libponyc-run/private-type-as-default-argument-in-public-function/lib/lib.pony @@ -0,0 +1,4 @@ +primitive _Private + +primitive Public + fun apply[T](v: (T | _Private) = _Private): None => None diff --git a/test/libponyc-run/private-type-as-default-argument-in-public-function/main.pony b/test/libponyc-run/private-type-as-default-argument-in-public-function/main.pony new file mode 100644 index 0000000000..44eede2517 --- /dev/null +++ b/test/libponyc-run/private-type-as-default-argument-in-public-function/main.pony @@ -0,0 +1,6 @@ +use lib = "lib" + +actor Main + new create(env: Env) => + let p = lib.Public.apply[U8]() + env.out.print(p.string()) From 5fd6c26f316c0f0a7de4ef18b8bee205b768d60e Mon Sep 17 00:00:00 2001 From: stefandd Date: Wed, 27 Jul 2022 21:10:39 +0200 Subject: [PATCH 5/7] Create 4167.md Adding release notes --- .release-notes/4167.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 .release-notes/4167.md diff --git a/.release-notes/4167.md b/.release-notes/4167.md new file mode 100644 index 0000000000..a7da9e8cc2 --- /dev/null +++ b/.release-notes/4167.md @@ -0,0 +1,22 @@ +## Fix for compiler segfault #4130 + +The compiler crash described in #4130 happened with code that attempted to call a method in a different package when this remote method used a package-private type as a default parameter. + +```pony +// In the "lib" pacakge + +primitive _Private + +primitive Public + fun apply[T](v: (T | _Private) = _Private): None => None + +// In main +use lib = "lib" + +actor Main + new create(env: Env) => + let p = lib.Public.apply[U8]() + env.out.print(p.string()) +``` + +It was decided that this code is valid Pony code, and this PR fixes the crash so that code like the above compiles. From cb3b9601815391e43096d623a1eb85037c585d8e Mon Sep 17 00:00:00 2001 From: stefandd Date: Thu, 28 Jul 2022 17:51:29 +0200 Subject: [PATCH 6/7] Update test/libponyc-run/private-type-as-default-argument-in-public-function/main.pony Co-authored-by: Borja o'Cook --- .../main.pony | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/libponyc-run/private-type-as-default-argument-in-public-function/main.pony b/test/libponyc-run/private-type-as-default-argument-in-public-function/main.pony index 44eede2517..41c4a74d2e 100644 --- a/test/libponyc-run/private-type-as-default-argument-in-public-function/main.pony +++ b/test/libponyc-run/private-type-as-default-argument-in-public-function/main.pony @@ -2,5 +2,4 @@ use lib = "lib" actor Main new create(env: Env) => - let p = lib.Public.apply[U8]() - env.out.print(p.string()) + lib.Public.apply[U8]() From 323e371f109ea2f839f389296e1e0a7fc9a4b6db Mon Sep 17 00:00:00 2001 From: stefandd Date: Sat, 30 Jul 2022 20:26:19 +0200 Subject: [PATCH 7/7] Update 4167.md --- .release-notes/4167.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.release-notes/4167.md b/.release-notes/4167.md index a7da9e8cc2..a95170d32c 100644 --- a/.release-notes/4167.md +++ b/.release-notes/4167.md @@ -1,4 +1,4 @@ -## Fix for compiler segfault #4130 +## Fix compiler crash related to using private types as default arguments The compiler crash described in #4130 happened with code that attempted to call a method in a different package when this remote method used a package-private type as a default parameter.