-
-
Notifications
You must be signed in to change notification settings - Fork 419
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
Fix for crash when methods with default private type params in remote packages are called #4167
Conversation
This attempts to fix issues ponylang#4130 and ponylang#4153. The issue was when a private type in another package was used as a default value in a method call. Fix to ponylang#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 ponylang#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.
…ult values" This reverts commit 527864d.
This attempts to fix ponylang#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 ponylang#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()) ```
@stefandd Can you add the minimal example as a runner test so that we can ensure that this doesn't break in the future? |
Hi @stefandd, The changelog - fixed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do. Release notes are added by creating a uniquely named file in the The basic format of the release notes (using markdown) should be:
Thanks. |
@ergl Will do. I am not familiar with the folder naming in libponyc-run but it is clearly meant to be problem-related and discernable. For this issue I have longish names like "out-of-package calls to methods with private-type default parameters". |
@stefandd Having long names is not a deal breaker. Perhaps something along the lines of |
Adding release notes
@ergl Since both the direction for the fix and the minimal example that is now a test runner came from you, it'd be very appropriate for you to be added to this PR. How can I do this? |
@stefandd I'm not aware of a way to do that, but don't worry about it. If you insist, you can add my github info to the commit message like this: https://stackoverflow.com/questions/7442112/how-to-attribute-a-single-commit-to-multiple-developers |
test/libponyc-run/private-type-as-default-argument-in-public-function/main.pony
Outdated
Show resolved
Hide resolved
…unction/main.pony Co-authored-by: Borja o'Cook <[email protected]>
Thanks. Looks like you were added with the last commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from some nits with the release notes, I think this looks good
This attempts to fix the compiler crash reported in #4130
The crash happens upon attempting to call a methods from a different package that uses a package-private type as default parameter. Since it has been decided to treat this as a bug instead of a missing error, this PR implements the fix suggested by @ergl (#4130 (comment)) , namely using
lookup_try()
instead of lookup() in call.c'scheck_partial_function_call()
since the former can be configured to permit private types.With this PR, the two examples below that crashed the compiler now both compile:
Original example:
Minimal example:
Co-authored-by: Borja o'Cook [email protected]