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

Fix for crash when methods with default private type params in remote packages are called #4167

Merged
merged 8 commits into from
Aug 2, 2022

Conversation

stefandd
Copy link
Contributor

@stefandd stefandd commented Jul 27, 2022

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's check_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:

// 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:

// 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())

Co-authored-by: Borja o'Cook ergl@users.noreply.github.com

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.
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())
```
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Jul 27, 2022
@ergl
Copy link
Member

ergl commented Jul 27, 2022

@stefandd Can you add the minimal example as a runner test so that we can ensure that this doesn't break in the future?

@ergl ergl added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Jul 27, 2022
@ponylang-main
Copy link
Contributor

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 .release-notes directory. We suggest you call the file 4167.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

@stefandd
Copy link
Contributor Author

stefandd commented Jul 27, 2022

@stefandd Can you add the minimal example as a runner test so that we can ensure that this doesn't break in the future?

@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".
Is this too much - do you have a suggestion how to call the folder, or does it truly not matter?

@ergl
Copy link
Member

ergl commented Jul 27, 2022

@stefandd Having long names is not a deal breaker. Perhaps something along the lines of private-type-as-default-argument-in-public-function ?

@stefandd
Copy link
Contributor Author

stefandd commented Jul 27, 2022

@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 stefandd changed the title Fix for crash involving the use of private types used as default values - 2nd attempt Fix for crash when methods in remote packages are called with default private types Jul 28, 2022
@stefandd stefandd changed the title Fix for crash when methods in remote packages are called with default private types Fix for crash when methods with default private type params in remote packages are called Jul 28, 2022
@ergl
Copy link
Member

ergl commented Jul 28, 2022

@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

…unction/main.pony

Co-authored-by: Borja o'Cook <ergl@users.noreply.github.com>
@stefandd
Copy link
Contributor Author

@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

Thanks. Looks like you were added with the last commit.

.release-notes/4167.md Outdated Show resolved Hide resolved
Copy link
Member

@ergl ergl left a 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

@jemc jemc merged commit 6c6fc2e into ponylang:main Aug 2, 2022
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Aug 2, 2022
github-actions bot pushed a commit that referenced this pull request Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants