Use #not_nil! in property! macro

[straight-shoota]

Refactor custom error messages non-nilable getters defined by property! and getter! macros, using #not_nil!(message) (introduced in #12797).

[straight-shoota]

The spec failures are surprising. Apparently value.not_nil! does not have the type as (effectively) value.nil? ? raise "" : value?

method Crystal::Generic#instance_type must return Crystal::GenericType but it is returning Crystal::ModuleType+

What's especially odd about this is that the implementation of property! and getter! had actually used not_nil! before, prior to the improvement of error messages in #8200.
So it seems something in between must have changed with the typing.

[Sija]

Maybe the self coming from Object#not_nil! gets for some reason turned into the Parent+ union? Since this was the previous behavior as you mentioned, this theory doesn't really make sense though...

[straight-shoota]

Yes, exactly that is the problem. The call not_nil! gets dispatched to GenericClassType#not_nil! and GenericModuleType#not_nil! which are different methods, although both are implemented by Object. They return self and the resulting type union GenericClassType | GenericModuleType which is summarized as virtual ModuleType+ (that's discussed #2661).

Reduced demonstration:

module GenericType; end

abstract class ModuleType; end
abstract class ClassType < ModuleType; end

class GenericClassType < ClassType
  include GenericType
end

class GenericModuleType < ModuleType
  include GenericType
end

x = GenericClassType.new.as(GenericType)

typeof(x)                  # => GenericType
typeof(x || raise "") # => GenericType
typeof(x.not_nil!)         # => ModuleType (ModuleType+)

And the compiler actually already behaved exactly the same in 0.31.0, the last release which used not_nil!. There were just no type restrictions on the generated methods, thus the type mismatch wouldn't lead to a compiler error.

class Foo
  property! foo : GenericType
end

typeof(Foo.new.foo) # GenericType (>= 0.32.0), ModuleType+ (<0.32.0)

So, essentially, this unexpected (?) behaviour belongs to the discussion of #2661.

The change of this PR is not essential. It was just supposed to clean up the code a bit by using the newly available helper not_nil!(String). But we can easily forgo and close it.

However, I'm also thinking this behaviour of not_nil! is undesired. It's supposed to remove the Nil type from a union. Any other change in the type is uncalled for. See #12846 for tracking this.