Regression in untyped `getter` macro

[straight-shoota]

The following code types on Crystal 1.15 without explicit type annotations, but it's broken in master. This is a regression from #15548.

class Bar
  class_getter default_foo do
    "foo"
  end

  getter foo do
    Bar.default_foo
  end
end

Bar.new.foo # Error: can't infer the type of instance variable '@foo' of Bar

The change in the body of the block variant of class_getter:

# Crystal 1.15
if (%value = @@{{var_name}}).nil?
  @@{{var_name}} = {{yield}}
else
  %value
end

# master (#15548)
if (%value = @@{{var_name}}).nil?
  ::Crystal.once(pointerof(@@__{{var_name}}_flag)) do
    @@{{var_name}} = {{yield}} if @@{{var_name}}.nil?
  end
  @@{{var_name}}.not_nil!
else
  %value
end

I suppose the additional logic there makes it impossible for the compiler to type the return value. Although, it's already a bit surprising that it managed to type it in the first place.

Interestingly, the assignment variant class_getter default_foo = "foo" doesn't type in 1.15, despite being theoretically simpler and therefore should be easier to type 🤷

This error was discovered in test-ecosystem for https://github.com/Sija/backtracer.cr/blob/d461ca301b6ea7a8ac1ada673eacee098e248047/src/backtracer/backtrace/frame.cr#L18

[straight-shoota]

The issue seems to be that the compiler cannot type the final .not_nil! call.
We could fix this by enhancing compiler support for typing not_nil!.

However, we might be able to solve it entirely in user code with an explicit cast instead.
I don't think there's any trivial way to remove Nil from the type, but we can use typeof({{ yield }}) to determine the intended return type based on the block type.

diff --git i/src/object/properties.cr w/src/object/properties.cr
index 2cbf1e034..4162e942f 100644
--- i/src/object/properties.cr
+++ w/src/object/properties.cr
@@ -342,7 +342,7 @@ class Object
             ::Crystal.once(pointerof(@@__{{var_name}}_flag)) do
               @@{{var_name}} = {{yield}} if @@{{var_name}}.nil?
             end
-            @@{{var_name}}.not_nil!
+            @@{{var_name}}.as(typeof({{ yield }}))
           else
             %value
           end

Unfortunately, this breaks when the block type is a pointer:

class Baz
  class_getter default_foo do
    Pointer(Void).null
  end

  getter foo do
    Baz.default_foo
  end
end

Baz.new.foo # Error: can't cast (Pointer(Void) | Nil) to Pointer(Void)

We use this in stdlib in Regex.match_context (class_getter match_context : LibPCRE2::MatchContext*).
As a workaround, we could consider keeping .not_nil! for typed getter. This makes stdlib and the original example type correctly.
But it would still be a regression for Pointer typed getters without an explicit return type.

[ysbaddaden]

This is an edge case of initializing an ivar from a class var that is lazily initialized and where both are untyped.

1.15 already fails for class_getter foo = "foo" because it does go through __crystal_once to initialize the global, so Bar.foo isn't easily typed and compilation fails to type @foo.

#15548 is just pushing that for lazily initialized class variables, too: either of @foo or Bar.foo must be typed, and Crystal explicitly states that at least @foo must be.

IMO: unless it's a too common occurrence that everything is going to break, this is an acceptable "regression" compared to the benefits of fixing #14905.

I would fix the backtracer shard (it's backward compatible) and see if anything else breaks in test-ecosystem.

[straight-shoota]

Casting with .as(typeof({{yield}})) still doesn't work for me: Error: can't infer the type of instance variable '@foo' of Bar

That seems odd. It definitely works for me.

git checkout a2af0264973af1febe045448c3a48dbd5e7837e5 # current master
git apply << EOF
diff --git i/src/object/properties.cr w/src/object/properties.cr
index 2cbf1e034..4162e942f 100644
--- i/src/object/properties.cr
+++ w/src/object/properties.cr
@@ -342,7 +342,7 @@ class Object
             ::Crystal.once(pointerof(@@__{{var_name}}_flag)) do
               @@{{var_name}} = {{yield}} if @@{{var_name}}.nil?
             end
-            @@{{var_name}}.not_nil!
+            @@{{var_name}}.as(typeof({{ yield }}))
           else
             %value
           end
EOF
bin/crystal eval << EOF
class Bar
  class_getter default_foo do
    "foo"
  end

  getter foo do
    Bar.default_foo
  end
end

Bar.new.foo
EOG

[straight-shoota]

1.15 already fails for class_getter foo = "foo" because it does go through __crystal_once to initialize the global, so Bar.foo isn't easily typed and compilation fails to type @foo.

Ah, I didn't realize that we already changed this in 1.15 and that's why it fails. So this is a regression between 1.14 and 1.15.

IMO: unless it's a too common occurrence that everything is going to break, this is an acceptable "regression" compared to the benefits of fixing #14905.

typeof({{ yield }}) should be a reasonable workaround for most use cases, if we opt to use not_nil! when the getter is typed. That would certainly be better than the current broken state.

see if anything else breaks in test-ecosystem.

That's the only failure (unless this failure shadows some other one which we'll only know when we have fixed it).

[ysbaddaden]

@straight-shoota I meant 1.15 and below, I assume all previous releases to be affected (they already used __crystal_once and we only changed the implementation behind the fun), but we could test.

[ysbaddaden]

Thanks @Sija for fixing this 🙇 Let's see if that unbreaks test-ecosystem or shadowed other use cases.

Maybe we can "hide" the feature behind preview_mt for the time being?

[jwoertink]

Will this affect instance getter too, or just the class_getter? I have untyped instance getter in Cable

https://github.com/cable-cr/cable/blob/5abfa6533ec8a8fe99cac0c6b518982746f9a50e/src/cable/server.cr#L36-L41

I wanted to add the type, but couldn't find a solid solution since the return value depends on the adapter you're using 😬

edit: nevermind, I just saw it's being reverted. I'll start looking in to adding a type on to this anyway.

[straight-shoota]

Instance getter should not be affected. We didn't change anything about that.
You can easily check with a nightly build of Crystal (or just a checkout of stdlib from master as there is no compiler change involved).

There's a revert PR but we might just patch up the current implementation with a workaround.

[straight-shoota]

Maybe we can "hide" the feature behind preview_mt for the time being?

That sounds like a good quick fix to keep moving. It'll give us time to figure out the best solution.