crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.47k stars 1.62k forks source link

Proc semantics are broken in 1.0 #10546

Open naqvis opened 3 years ago

naqvis commented 3 years ago

1.0 changed the semantics of Proc and now its treated as block. Possible regression of? https://github.com/crystal-lang/crystal/pull/10263

Below snippet used to work in previous versions, but no longer works in 1.0

alias Foo = -> String

Foo.new {
  return "foo" # => Error: can't return from captured block, use next
}

if this was intended change for 1.0 then this also breaks using of looping constructs inside Procs, as next in looping construct isn't same as exiting from block.

Contrived example (works in Previous versions), but would be troublesome(if not impossible) with whole of boiler-plate code to achieve same in 1.0

alias Foo = -> String

def foo
  Foo.new {
  loop do
    x = 1
    while x == 1
      return "fooo" # => Error: can't return from captured block, use next
    end
    return "bar"
  end
  }
end

p! foo.call
asterite commented 3 years ago

@naqvis is this comment what you are referring to? https://github.com/crystal-lang/crystal/pull/10263#issuecomment-781653418

asterite commented 3 years ago

Also please include what errors you get in code snippets you post, because otherwise every one of us has to try it to see what's the error, if any.

naqvis commented 3 years ago

@naqvis is this comment what you are referring to? #10263 (comment)

Thanks Ary, point I am trying to make is Proc according to reference guide is a function pointer, so semantically it should behave like normal method/function definition where one is allowed to return out of the function at any point. I understand return still works with ->, but isn't that a syntactic sugar for Proc?

Also please include what errors you get in code snippets you post, because otherwise every one of us has to try it to see what's the error, if any.

Updated initial post with error message.

asterite commented 3 years ago

My follow up comments on that issue follow the linked comment (for example https://github.com/crystal-lang/crystal/pull/10263#issuecomment-781654998 and https://github.com/crystal-lang/crystal/pull/10263#issuecomment-781673037 ). I think inside a Proc you should use next, and return should always be a syntax error. But it's probably too late to change that now.

naqvis commented 3 years ago

For time-being if we ignore about the keyword to return out of the Proc, inconsistency between -> and Proc still exists in 1.0. Question would be shouldn't -> and Proc behave similarly?

Below snippet works.

-> {
  return "foo"
}
straight-shoota commented 3 years ago

I guess the discussion about this was going to happen sooner or later anyways.

I'll try to summarize the main issues in the problem space:

  1. Let's consider foo { return true }. If foo yields, the block is executed in the outer scope and returns from that. And it errors if the expression is outside a method. But if foo captures the block it creates an ambiguity: Strictly thinking, return applies to the proc function directly. But for the similarity with a yielded block it could also be expected to apply to the external method scope. Whether a block literal is captured or yielded is not visible at the call site. It depends on the implementation of foo. And different return behaviours are confusing.

  2. The alternative for returning from a proc scope is next. That works with any block. But the next clause is overloaded for loop constructs. It's used to skip an iteration in while and nested blocks like those for enumeration methods. And next does not have any semantics to target which scope is to be exited. That's the big benefit of return because it applies to the outer method scope. Thechnically this is already problematic with nested iteration contexts by themselves, but this doesn't seem to be a huge issue in practice. But between procs and loop constructs, there's more chance for collision. And it is impossible to directly express exit from a scope that is not the inner most with next (or break for that matter).

  3. The third problem is the similarity (or rather dissimialarity) between captured blocks and proc literals. The latter currently allows return and does not accept next. For captured blocks it's exactly the opposite. But both serve exactly the same purpose, just the syntax is a bit different. This is probably the easiest to find a solution if we just apply the removal of return to proc literals as well (and in turn introduce next). It could even be reasoned about whether it actually makes sense to have both. Since #10263 Proc.new is trivially implemented as a simple captured block and it is convenient for use with alias types. And proc literals probably have enough reason to exist as a language feature for use with C bindings.

toddsundsted commented 3 years ago

i'd love to see syntax that works in both 0.36.1 and 1.0.0. i haven't been able to find a way, without a lot of rewriting, to fix procs being used as callbacks to C functions that broke with the recent release (that depended on being able to return) that work with both versions.

naqvis commented 3 years ago

Do we have any workaround (without whole lot of boilerplate) for returning from loops inside a Proc? as next inside a loop is going to continue the iteration other than returning from block. I understand break value inside loop works, but this don't work in while until https://github.com/crystal-lang/crystal/pull/10566 is merged.

asterite commented 3 years ago

Assing to a variable, then break.

HertzDevil commented 3 years ago

IMO the type restrictions for -> parameters should be optional under more circumstances, then we don't need to go through Proc.new every time (and we might not even need an alias). Crystal already does this for -> literals as arguments of lib funs:

lib LibFoo
  alias Bar = LibC::Int, LibC::Char -> Bool

  fun foo(cb : Bar)
end

module M
  class_property! u : Int32
  class_property! v : UInt8
end

LibFoo.foo(->(x, y) do # notice lack of parameter restrictions here
  M.u = x # okay, x is inferred to be a LibC::Int (Int32)
  M.v = y # okay, y is inferred to be a LibC::Char (UInt8)
  true
end)

Perhaps something similar to autocasting can be done here for the case of regular defs.

straight-shoota commented 3 years ago

Lib functions have neither overloads nor dynamic dispatch, so there's ever only one matching proc signature. That makes it trivial to match the types to the literal.

It could still be a viable option to try some autocasting for Crystal methods. I suppose it's probably not very common to have overloads for different proc types and this "autocasting" could work for a lot of use cases. But I'd also fear it'd be kind of awkward when it breaks just because another overload is added, for example.