Align Proc.new(&block)'s behaviour with other captured blocks

[HertzDevil]

Defines Proc.new(&block) in the standard library instead of hard-coding its behaviour in the compiler, c.f. #10001 (comment).

Resolves #9813. Although unconfirmed, this constructor should also benefit from #10251.

There is still compiler magic involving Proc.new(&block) in the cleanup transformer:

• Direct uses of this constructor inside a lib fun call, whether through an alias or not, still check for closured vars during compile-time. This is already done for -> literals; the reason Proc.new used to work is because the compiler internally rewrote these calls to -> literals.
• As per request, all definitions of Proc.new(&block) must match the standard library's definition exactly; that is, it must take only a block argument matching the Proc's instantiated type, return that block, and do nothing else. This shouldn't affect any existing code since the constructor is definitely not meant to be redefined.

Additionally, the compiler will now check for closured vars in any captured block inside a lib fun call as long as the enclosing method returns the captured block and does nothing else. This applies for Proc.new(&), but also cases like the following are detected:

def capture(&block : -> Int32) # checked, body consists only of `block`
  block
end

lib LibC
  fun foo(x : ->)
end

a = 1
LibC.foo(capture do            # Error: can't send closure to C function (closured vars: a)
  a
end)

This means there is now far less compiler magic involving Proc.new(&block) (probably none after this PR).

[straight-shoota]

Can we merge only the new Proc constructor for 0.36.0 and the compiler change after that? Otherwise 1.0-dev compiler would not be able to compile 0.36.0 stdlib, right?

[asterite]

@straight-shoota I think both can go at the same time. A compiler compiling this new compiler has the hardcoded logic for that.

[straight-shoota]

Yeah okay, master compiler already can't compile 0.35.1 anymore.

In the future I would suggest to move slower with changes affecting the interface between stdlib and the compiler and always have one release in between which works in both directions.

[asterite]

A new compiler isn't expected to compile an older compiler.

[straight-shoota]

No, that's not necessary. But it would be nice if a new compiler would be somewhat compatible with stdlib of the previous release.

[bcardiff]

That is necessary because the previous compiler is used with the current std-lib to build the new version.
That is why new language features can't be used directly. Or if used need to be fenced with a compare_versions macro.

[straight-shoota]

Yeah well, as I said, current master does not compile 0.35.1 stdlib.

[bcardiff]

Ok, I see want you mean, but in what scenario you would need for that to work?

[straight-shoota]

Bug hunting 😄
For example #10088 behaves differently with 0.35.1 and master compiler. I'd like to track stdlib changes against master compiler but I can't just bisect because it fails to build for older commits.

[straight-shoota]

It would be great to at least clearly document the migration process in the PR.

[asterite]

I don't think there's any migration process?

A hardcode method in Proc is removed, but then an equivalent method in std is added.

[straight-shoota]

Oh, sorry I confused this PR with a breaking change.

[bcardiff]

A side effect of this change is that, I guess reasonable, we lost the possibility to use return in the captured blocks

This used to work:

alias P = (Int32) -> Int32

p = P.new do |n|
  return 0 if n < 0
  n
end

But after this change the user will see

In foo.ign.cr:4:3

 4 | return 0 if n < 0
     ^
Error: can't return from captured block, use next

So is either reworking the code to avoid using return or using the proc literal notation which is still valid after this PR.

p = ->(n : Int32) {
  return 0 if n < 0
  n
}

If someone thinks that this going in the wrong direction we can revert. But I think it make sense and we are still improving the consistency.

[asterite]

Interesting!

This is actually going in the right direction. return inside a captured block should always give an error. That this works is a bug:

p = ->(n : Int32) {
  return 0 if n < 0
  n
}

To return from a block one should always use next.

The reason is that return is always associated with the enclosing method, never a block or proc (like in Ruby).

[bcardiff]

What is the problem with allowing return in proc literals?

[asterite]

What is the problem with allowing return in proc literals?

I think it's just confusing. Consider this in Ruby:

def foo
  ->(n) {
    return 10 if n < 0
    n
  }.call(-1)

  proc { |n|
    return 20 if n < 0
    n
  }.call(-1)

  bar do |n|
    return 30 if n < 0
    n
  end

  30
end

def bar
  yield -1
end

p foo

What do you expect the return value to be?

Now try it and tell me whether it's intuitive.

The problem is that when you see return you relate it to the enclosing method, in this case foo. If the meaning of return changes depending on whether you are inside a block, a captured block or a proc literal, it makes things confusing. We should try to be uniform here in my opinion.

[HertzDevil]

Crystal doesn't seem to support next inside Proc literals yet:

# Crystal
foo = ->(n : Int32) do
  next 0 if n < 0 # Error: invalid next
  n
end

foo = Proc(Int32, Int32).new do |n|
  next 0 if n < 0 # okay after this PR (good!)
  n
end

foo.call(-1) # => 0

So @bcardiff if you replace return with next in that mentioning issue then it should also work (although this will definitely break all pre-1.0.0 compilers).

The problem is both Crystal and Ruby seem to behave as if returns inside -> are rewritten into nexts.

[bcardiff]

I read proc literal as lambdas. So I would expect to be able to use return as with any def. But I see the benefits on keeping the limitation. Specially in contexts where the proc literal body might be generated by a macro expansion.