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

Ivar value within struct not changing when using &-> #10035

Closed
Blacksmoke16 opened this issue Dec 5, 2020 · 13 comments
Closed

Ivar value within struct not changing when using &-> #10035

Blacksmoke16 opened this issue Dec 5, 2020 · 13 comments

Comments

@Blacksmoke16
Copy link
Member

Blacksmoke16 commented Dec 5, 2020

Not sure if this is intended or not but after #9972, changing the value of an ivar within a struct using &-> proc notation causes the ivar's value to not actually change.

struct Container
  getter answer : Bool = false

  def add_value(value : Int32)
    if value.even?
      @answer = true
    end
  end

  def add_values(values : Array(Int32))
    values.each &->add_value(Int32)
  end
end

c = Container.new
values = [2]

c.add_values values

pp c

On 0.35.1: Container(@answer=true), while on master: Container(@answer=false).

I believe with this, Procs don't have any more bugs (that I am aware of, so this is probably a lie stuck_out_tongue )

@asterite You guessed correctly haha

@asterite
Copy link
Member

asterite commented Dec 5, 2020

A copy of the struct is passed, please don't do that. I won't revert my change. Just use a regular block.

@asterite
Copy link
Member

asterite commented Dec 5, 2020

we should document the behavior of procs, basically saying what they rewrite to.

In fact, I would like to remove this anti feature because it's redundant, except for taking the pointer of C functions

@asterite
Copy link
Member

asterite commented Dec 5, 2020

and this:

values.each &->add_value(Int32)

Could be this:

values.each &add_value(&1)

and that would work well because it's just a regular block

I don't understand why you (or others) use the more complex syntax. What's wrong with using a block with an argument and passing it inside the block?

@asterite
Copy link
Member

asterite commented Dec 5, 2020

What I'm saying is, I never use that proc notation myself. It feels off.

@jhass
Copy link
Member

jhass commented Dec 5, 2020

I'm not sure anybody realized call &other_call is a thing, I didn't until now :D Looks like it's post-0.35?

@asterite
Copy link
Member

asterite commented Dec 5, 2020

What do you mean?

@jhass
Copy link
Member

jhass commented Dec 5, 2020

Could be this:

values.each &add_value(&1)

Where does this syntax come from? It doesn't work on 0.35: https://carc.in/#/r/a26o (also not in the argless version)

@asterite
Copy link
Member

asterite commented Dec 5, 2020

It's in my ideal version of the language, where #9218 and many others are merged.

@jhass
Copy link
Member

jhass commented Dec 5, 2020

Ah, then

I don't understand why you (or others) use the more complex syntax. What's wrong with using a block with an argument and passing it inside the block

was worded a bit misleading :) At least to me it sounded like it's an alternative already.

@asterite
Copy link
Member

asterite commented Dec 5, 2020

I meant, instead of:

values.each &->add_value(Int32)

write this:

values.each { |value| add_value(value) }

It's a bit longer, yes... but someone reading the code won't stop and wonder what's that &-> and why types need to be specified there. You'll also need to change that type if the underlying method changes. The block syntax is longer but simpler and more reliable.

@Blacksmoke16
Copy link
Member Author

A copy of the struct is passed, please don't do that. I won't revert my change. Just use a regular block.

That makes sense. I'm not too bothered by it, in my actual use case this made me notice the type should have been a class anyway (missed updating this one). At the least I'd maybe mark #9972 as a breaking change given the semantics of &-> aren't the same as before anymore.

@asterite
Copy link
Member

asterite commented Dec 5, 2020

Makes sense. Done!

@Blacksmoke16
Copy link
Member Author

@asterite 👍

Going to close this then as it's intended behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants