Crystal: can't read other field values in the record constructor in 0.35.0

[maiha]

It is very useful that we can read other field values in the record constructor.

record Foo, a : Int32, b : Int32 = a + 1

However, this does not work with 0.35.0. Is this a intended change?

$ for v in 0.27.2 0.34.0 0.35.0; do echo -n "$v "; crenv local $v && crystal test.cr && echo OK; done
0.27.2 OK
0.34.0 OK
0.35.0 Showing last frame. Use --error-trace for full trace.

In test.cr:1:36

 1 | record Foo, a : Int32, b : Int32 = a + 1
                                        ^
Error: undefined local variable or method 'a' for Foo.class

FYI

It seems to work for class constructors.

class Foo
  getter a : Int32
  getter b : Int32
  def initialize(@a, @b = a + 1)
  end
end

0.27.2 OK
0.34.0 OK
0.35.0 OK

Best regards,

[asterite]

Hi! Yeah, it should work. This is easy to fix:

diff --git a/src/macros.cr b/src/macros.cr
index b09ad1db6..e426f022c 100644
--- a/src/macros.cr
+++ b/src/macros.cr
@@ -63,7 +63,7 @@ macro record(name, *properties)
       {% if property.is_a?(Assign) %}
         getter {{property.target.id}}
       {% elsif property.is_a?(TypeDeclaration) %}
-        getter {{property}}
+        getter {{property.var}} : {{property.type}}
       {% else %}
         getter :{{property.id}}
       {% end %}

Please feel free to send a PR (I don't have time right now).

[asterite]

It's also possible this was changed in some other PR

[asterite]

This PR broke it: https://github.com/crystal-lang/crystal/pull/9063

I think that PR should be reverted, it's a bit strange to use the default value in the getter... exactly because it doesn't work with more complex expressions, and because it would be repeating the value.

[asterite]

Actually, the use case of #9008 seems valid.

I think one of the cases need to make a compromise and write the struct without using macros.

[Blacksmoke16]

I think we can support both:

diff --git a/src/macros.cr b/src/macros.cr
index b09ad1db6..d383327d4 100644
--- a/src/macros.cr
+++ b/src/macros.cr
@@ -63,7 +63,7 @@ macro record(name, *properties)
{% if property.is_a?(Assign) %}
getter {{property.target.id}}
{% elsif property.is_a?(TypeDeclaration) %}
-        getter {{property}}
+        getter {{property.var}} : {{property.type}}{% if !property.value.is_a?(Nop) && !property.value.is_a?(Call) %} = {{property.value}} {% end %}
{% else %}
getter :{{property.id}}
{% end %}

Only add a default to the getter if there is a value, and its not a Call. Adding this extra spec passes, as well as the current specs:

diff --git a/spec/std/record_spec.cr b/spec/std/record_spec.cr
index f4034e9c2..27c5e0985 100644
--- a/spec/std/record_spec.cr
+++ b/spec/std/record_spec.cr
@@ -25,6 +25,8 @@ private record CustomInitializer, id : Int32, active : Bool = false do
end
end

+private record GetterInInitializer, a : Int32, b : Int32 = a + 1
+
describe "record" do
it "defines record with type declarations" do
ary = [2, 3]
@@ -85,4 +87,11 @@ describe "record" do
it "uses the default values on the ivars" do
CustomInitializer.new(__id: 10).active.should be_false
end
+
+  it "allows using getters within the initializer" do
+    GetterInInitializer.new(1).tap do |obj|
+      obj.a.should eq 1
+      obj.b.should eq 2
+    end
+  end
end

[asterite]

It's clever, but I'm not sure it's sound. For example if my default value is Foo.bar then it won't work.

I think the condition is "if the default value doesn't use the other values" (other properties), but it's a bit tricky to do.

I think we should close this issue and recommend not using record in this case. Writing a struct by hand with such behavior is just a couple of lines anwyay.