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

Arguing refcaps #4566

Closed
hovsater opened this issue Dec 5, 2024 · 7 comments
Closed

Arguing refcaps #4566

hovsater opened this issue Dec 5, 2024 · 7 comments
Labels
discuss during sync Should be discussed during an upcoming sync needs discussion Needs to be discussed further

Comments

@hovsater
Copy link

hovsater commented Dec 5, 2024

In this Playground example (also attached below), table have been captured by the lambda. The code does not compile, but I'm pretty sure it should. The issue was also shortly discussed on Zulip prior to this issue.

Playground code

use "collections"
use "itertools"

class Foo is (Equatable[Foo])
  let _numbers: Set[U32]
  
  new create(numbers: Set[U32]) =>
    _numbers = numbers
    
actor Main
  new create(env: Env) =>
    let table: Map[U32, Set[U32]] = table.create()
    let container: Array[Foo] =
      Iter[U32]([as U32: 1; 2; 3].values())
        .map[Foo]({(n: U32) => Foo(table.get_or_else(0, Set[U32]))})
        .collect(Array[Foo])
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Dec 5, 2024
@SeanTAllen
Copy link
Member

Ok so looking at this, there's a few things going on with what @hovsater is trying to do:

First this works:

use "collections"

class Foo is (Equatable[Foo])
  let _numbers: Set[U32]

  new create(numbers: Set[U32]) =>
    _numbers = numbers

class Bob
  let t: Map[U32, Set[U32]] = t.create()

  fun ref apply(n: U32): Foo =>
    let empty = Set[U32]
    let a = t.get_or_else(n, empty)
    Foo(a)

But that isn't what @hovsater's code desugars to effectively. His lambda is creating a fun box apply and then we get the error from get_or_else as the "else" and return type from it are this->V.

You can see a similar error from this simpler code:

use "collections"

class Foo is (Equatable[Foo])
  let _numbers: Set[U32]

  new create(numbers: Set[U32]) =>
    _numbers = numbers

class Bob
  let t: Map[U32, Set[U32]] = t.create()

  fun apply(n: U32): Foo =>
    let empty = Set[U32]
    let a = t.get_or_else(n, empty)
    Foo(a)

But that leads to a different issue with itertools which can be seen in this slightly simplified example:

use "collections"
use "itertools"

class Foo is (Equatable[Foo])
  let _numbers: Set[U32]

  new create(numbers: Set[U32]) =>
    _numbers = numbers

actor Main
  new create(env: Env) =>
    let container: Array[Foo] =
      Iter[U32]([as U32: 1; 2; 3].values())
        .map[Foo](
          object ref
            let t: Map[U32, Set[U32]] = Map[U32, Set[U32]]
            fun ref apply(n: U32): Foo =>
              let empty = Set[U32]
              let a = t.get_or_else(n, empty)
              Foo(a)
          end)
        .collect(Array[Foo])

@SeanTAllen SeanTAllen added the needs discussion Needs to be discussed further label Dec 5, 2024
@SeanTAllen
Copy link
Member

@jemc I won't be around for the next sync, but I hope that I've left enough crumbs for this to start being discussed more fully.

@SeanTAllen SeanTAllen changed the title Lambda capture causes type mistmatch Arguing refcaps Dec 5, 2024
@SeanTAllen
Copy link
Member

@hovsater noted in Zulip that map_stateful works. I am not very educated about Itertools. Perhaps that was created for situations such as this?

@SeanTAllen
Copy link
Member

i discussed with @jemc who knows Iter much better than I. This issue and similar is indeed why map_stateful exists. It is the proper solution.

@ponylang-main ponylang-main added discuss during sync Should be discussed during an upcoming sync and removed discuss during sync Should be discussed during an upcoming sync labels Dec 6, 2024
@hovsater
Copy link
Author

hovsater commented Dec 6, 2024

Alright. I'm still not entirely following why we need map_stateful here. Would an explanation be possible? Perhaps for the sole reason to understand reference capabilities better.

@SeanTAllen
Copy link
Member

SeanTAllen commented Dec 6, 2024

Let's chat at an upcoming Office Hours (I wont be there next week). I laid everything out above so I think it is an issue of walking through it together and answering questions.

I'll be there on the 16th.

@jemc
Copy link
Member

jemc commented Dec 6, 2024

Here's a quick summary from my end:

  • map takes a fun box apply object and map_stateful takes a fun ref apply object
  • since you're using map, your function is box / read-only, and everything it sees in its closure environment is via box viewpoint-adaptation (i.e. everything is seen as read-only) (see also this table on viewpoint adaptation)
  • You cannot obtain a mutable Set[U32]'ref from a read-only Map[U32, Set[U32]'ref]'box because of viewpoint adaptation - again, it's read-only all the way down
  • you need map_stateful because it will give you a read/write view of its closure environment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss during sync Should be discussed during an upcoming sync needs discussion Needs to be discussed further
Projects
None yet
Development

No branches or pull requests

4 participants