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

Parser: allow ->@ivar.foo and ->@@cvar.foo #9268

Merged
merged 2 commits into from
Jun 1, 2020

Conversation

makenowjust
Copy link
Contributor

Fixed #9239

Now, we can compile such a code:

class Foo
  def initialize
    @foo = "foo"
    proc = ->@foo.upcase
    p proc.call # => "FOO"
  end
end

Foo.new

Copy link
Member

@jhass jhass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems simple enough

@makenowjust
Copy link
Contributor Author

ping.

@jhass jhass added this to the 0.35.0 milestone May 27, 2020
@bcardiff
Copy link
Member

If we are allowing a new syntax, or supported case, we should add a spec regarding its semantics. So we would need something in spec/compiler/semantic/proc_spec.cr.

In particular what happens if the ivar or cvar changes values between the proc creation and the invoke?

@makenowjust
Copy link
Contributor Author

makenowjust commented May 27, 2020

The value (not reference) of ivar is captured. It is same as usual ->var.meth syntax semantics.

class Foo
  def initialize
    @foo = "foo"
    proc = ->@foo.upcase
    @foo = "bar"
    p proc.call # => "FOO"
  end
end

Foo.new

@makenowjust
Copy link
Contributor Author

Specs are added.

@bcardiff
Copy link
Member

I'm hesitant if the semantic should be to not capture the ivar value, but the self value.

The semantic of this pr can be achieved by assigning the ivar to a local variable.

class Foo
  def initialize
    @foo = "foo"
    foo = @foo
    proc = ->foo.upcase
    puts proc.call
  end
end

The semantic of capturing self is equivalent to have proc to an instance method and a forwarding method.

class Foo
  def initialize
    @foo = "foo"
    proc = ->self.upcase
    puts proc.call
  end

  def upcase
    @foo.upcase
  end
end

I constantly read @ivar as this->ivar, so that's why I expect to capture self instead of the ivar value.

Having the latter semantics in ->@ivar.foo will allow us to drop the need of the forwarding methods.

I think is also more consistent with how the typing of ivars works. Asuming that it's value can be changed at any time.

Maybe @hugopl can share a bit more of the background story that lead to the need for this construct.

@asterite
Copy link
Member

@bcardiff Don't you also find this confusing?

foo = "hello"
proc = ->foo.upcase
foo = "bye"
proc.call # => "HELLO"

That is, I think we are talking about a different problem than what this PR is trying to solve.

@bcardiff
Copy link
Member

Slightly, but understandable.
The presence of @ to me makes the difference IMO.

@hugopl
Copy link
Contributor

hugopl commented May 29, 2020

Maybe @hugopl can share a bit more of the background story that lead to the need for this construct.

The need of this construct was just because feels natural to expect the language to support ->@var.method if it supports ->var.method.

To be honest, I thought the behavior of proc = ->foo.upcase would be the same of proc = ->{ foo.upcase }, i.e. just a syntax sugar. Hopefully my use cases didn't change the local variable, so I had no problems.

i.e. I would also expect ->@var.method to capture the self, not the value of @var.

@makenowjust
Copy link
Contributor Author

Currently var, Klass and Const is only allowed as receiver of ->foo.method syntax. Then, I guess, the first implementor of this syntax (perhaps @asterite) planned to unify semantics between var case and Klass/Const cases because Klass and Const cannot get its reference generally.

It seems natural that ->foo.method is same as -> { foo.method }.


@asterite

That is, I think we are talking about a different problem than what this PR is trying to solve.

I think so too really. Thank you.

@waj
Copy link
Member

waj commented May 29, 2020

If ->foo.upcase and ->{ foo.upcase } were exactly the same, the former syntax shouldn't exist at all then. The idea is to capture a proc to call a function on an object. It doesn't matter if the variable changes the value or gets out of scope. The same applies for instance variables. If you want to capture self, you can easily write ->{ @foo.upcase }. I think the semantics proposed in this PR, of capturing the value and not the variable itself is correct.

@waj
Copy link
Member

waj commented May 29, 2020

That is, I think we are talking about a different problem than what this PR is trying to solve.

I don't think so, because both cases have similar syntax and then should have similar semantics.

Also note that if ->foo.bar was just a shorter for ->{ foo.bar }, and ->@foo.bar just a shorter for ->{ @foo.bar }, then the current behaviour (capture the value, not the variable) would be more complicated to obtain.

@hugopl
Copy link
Contributor

hugopl commented Jun 1, 2020

If ->foo.upcase and ->{ foo.upcase } were exactly the same, the former syntax shouldn't exist at all then.

Makes sense, I think the language reference should be more clear about this BTW, the docs just say:

A proc can optionally specify a receiver:

str = "hello"
proc = ->str.count(Char)
proc.call('e') # => 1
proc.call('l') # => 2

@bcardiff bcardiff merged commit cdc9829 into crystal-lang:master Jun 1, 2020
@makenowjust makenowjust deleted the fix/9239-proc-syntax branch June 1, 2020 18:01
makenowjust added a commit to makenowjust/crystal that referenced this pull request Jun 2, 2020
Now, `->var.foo` is just syntax sugar of `-> { var.foo }`.

It is PoC and an objection against @waj's comment.
crystal-lang#9268 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proc syntax sugar thrown a parser error if used with member variable
8 participants