Disallow typeof in type restrictions

[asterite]

Fixes #2052
Fixes #5189

Type restrictions in methods are currently lazily evaluated. That means that this doesn't give an error right now:

def foo(x : DoesntExist)
end

It only gives an error when you try to invoke it.

Eventually we'd like type restrictions to be solved as soon as found, to avoid confusions like this: #4897 . In that example, because BigRational didn't exist when defining the method, it got in an incorrect order in the overload list.

Because methods are defined and solved before "main code" is analyzed (method calls, etc.), typeof can't be used in type restrictions, just like it can be used in type declarations for instance variables.

Note that because we aren't yet solving restrictions as soon as found, the error you will get when using typeof here will only happen when you try to invoke the method, just like in the example above. Eventually this will be fixed too.

[RX14]

"errors"

[asterite]

Thanks. Fixed with an amend and force push.

[RX14]

ditto

[RX14]

I'm preliminarily for this PR, it seems like a sane change, but i've never used this construct so i'm not sure what the possible usecases in the wild are.

[asterite]

It doesn't matter what the use cases are, nor there's a point discussing in favor of keeping this feature. It simply shouldn't work and will anyway stop working in the future once we don't lazily solve type restrictions.

[makenowjust]

I agree with this totally because typeof restriction does not make a sense if restriction type is resolved before MainVisitor phase, and I think this early resolving is right way.

However this PR is not perfect. typeof restriction for block arg is allowed for now. This example (refined version of #2052) still works:

class Foo
  def foo
    1
  end

  def bar(&callback : typeof(self.foo) ->)
    yield foo
  end
end

Foo.new.bar do |x|
  p x
end

These line should be fixed:

crystal/src/compiler/crystal/semantic/call.cr

Line 711 in 1ac8880

tuple_type = lookup_node_type(match.context, input.exp)

crystal/src/compiler/crystal/semantic/call.cr

Line 721 in 1ac8880

arg_type = lookup_node_type(match.context, input)

crystal/src/compiler/crystal/semantic/call.cr

Line 733 in 1ac8880

block_arg_type = lookup_node_type(match.context, block_arg_restriction).remove_typedef

[asterite]

@makenowjust 🦅 👀 !! Thanks! I fixed those too now.

[makenowjust]

@asterite I'm not sure lookup_type for TypeOf is really needed. Because normal context typeof is resolved by MainVisitor.

[asterite]

What do you mean?

[makenowjust]

@asterite These lines may be replaced by simple node.raise "..." like Type#restrict for TypeOf:

crystal/src/compiler/crystal/semantic/type_lookup.cr

Lines 356 to 374 in ea4187c

	def lookup(node : TypeOf)
	unless @allow_typeof
	if @raise
	node.raise "can't use 'typeof' here"
	else
	return
	end
	end
	meta_vars = MetaVars{"self" => MetaVar.new("self", @self_type)}
	visitor = MainVisitor.new(program, meta_vars)
	expressions = node.expressions.clone
	begin
	expressions.each &.accept visitor
	rescue ex : Crystal::Exception
	node.raise "typing typeof", inner: ex
	end
	program.type_merge expressions
	end

[asterite]

Mmm... I'm still not sure I understand. But you can try to send a different PR, or show me the diff you want.

[paulcsmith]

@asterite FWIW I don't mind the removal of typeof in type restrictions, but I love that method overloads are lazily evaluated because it makes requiring files much more flexible w/o forward declarations. It seems to me that forward declarations would work, but might be less than ideal.

Would it be possible to store the original position of the overloaded method and then put it in that order once the type is resolved? That way we don't need forward declarations and you don't get weird issues like in: #4897

Not sure how hard that would be for the compiler, but it would make using Crystal extra nice IMO :D

[Papierkorb]

This change makes require "./foo/*" and friends utterly useless. Thus, we fall back to that awful manually-require-everything-on-your-own of ruby. This is no good.

Forward declarations? Is this C?

Why can't there be a Sema visitor which checks that all types are reachable afterwards?

[asterite]

To answer the questions, the main issue is macros. Macros can run at the same time methods are defined. So for example you can have:

class Foo
  def foo(x : Int32)
  end

  def foo(x : SomeType)
  end
end

{% for method in Foo.methods %}
  puts {{method.stringify}}
{% end %}

The above prints:

def foo(x : Int32)
end
def foo(x : SomeType)
end

Is that correct? Well, if we have:

alias SomeType = Int32

class Foo
  def foo(x : Int32)
  end

  def foo(x : SomeType)
  end
end

{% for method in Foo.methods %}
  puts {{method.stringify}}
{% end %}

Then we get:

def foo(x : SomeType)
end

because the second method redefines the first one.

Move the alias after the definition of class Foo and you get a different result. But the macro code is run right after the class is defined...!

So the only solution to this is to solve types and method as quickly as possible, and that means solving types in method declarations because we need to know if a method redefines another one, or whether a method is "stronger" than another to put it before others for multiple dispatch.

So this issue is not that simple.

I personally don't think forward declarations are that hard to deal with. You just write:

class Foo; end

and that's it. The only issue is having to do that when Foo inherits some other class, you'll have to repeat that (or require that class from that file, probably simpler).

I think the language is too complex, maybe without a good reason...

[Papierkorb]

Then the actual bug is that the compiler doesn't check for this error case once the aliases are resolved.

[asterite]

What error case?

[Papierkorb]

class Foo
  def foo(x : Int32)
  end

  def foo(x : SomeType)
  end
end

alias SomeType = Int32

Should give an error akin to

Aliasing SomeType to Int32 creates ambiguous methods.  I can't distinguish between these methods:
  - Foo#foo(x : Int32)
  - Foo#foo(x : SomeType) is same as Foo#foo(x : Int32)

[asterite]

Oh, I see. Well, if someone wants to implement that whole change, please send a PR :-)

It should also take into account simple types vs. union types.

[asterite]

Ping