Thanks but this is wrong, typeof(raise("").foo) should have the NoReturn type

If typeof(Foo) has type Foo:Class, then surely typeof(raise "") has type NoReturn:Class and the same by extension for typeof(raise("").foo).

@RX14 That's correct, but that's not what this PR is doing.

@asterite Updated. Okay?

@makenowjust I think in FixEmptyTypes visitor we just have to set the type to no_return if the type is missing.

@asterite I'll revert two commits (2c61942, 35893c9) because I think now CleanupTransformer is only way to set no_return to typeof missing types. Whether if typeof(exp) needs type, depends this typeof is executed on runtime, and it is decided by CleanupTransformer.

Sorry, but I don't know where did you get the idea that typeof(raise("")) should be NoReturm:Class instead of NoReturn. Look: https://carc.in/#/r/2www

p typeof(raise "") # => NoReturn

Similarly, if you have typeof(1) that is Int32 and in the compiler spec you'd expect to check against program.int32, not against program.int32.class.

The last commit you had was correct except it had to return NoReturn instead of NoReturn:Class. Doing it in CleanUpTransformer it more complex and involves more code. Plus, the node is already bound to its expressions, so I don't understand why your code makes it work, probably a coincidence.

@asterite But, we get this error:

p typeof(raise("")).foo

# Error in foo.cr:1: undefined method 'foo' for NoReturn:Class
# 
# p typeof(raise("")).foo
#                     ^~~
#

And p Int32.foo produces Error in line 1: undefined method 'foo' for Int32:Class.

It's because the foo method is looked up in the class of the receiver.

Similarly, if you have typeof(1) that is Int32 and in the compiler spec you'd expect to check against program.int32, not against program.int32.class.

require "./spec/spec_helper"

it "assert typeof(1) against int32" do
  assert_type(%(typeof(1))) { int32 }
  
end

it "assert typeof(1) against int32.metaclass" do
  assert_type(%(typeof(1))) { int32.metaclass }
end

$ ./bin/crystal run spec.cr
F.

Failures:

  1) assert typeof(1) against int32
     Failure/Error: result.type.should eq(expected_type)

       Expected: Int32
            got: Int32:Class

     # spec/spec_helper.cr:40

Finished in 259.35 milliseconds
2 examples, 1 failures, 0 errors, 0 pending

Failed examples:

crystal spec spec.cr:3 # assert typeof(1) against int32

Hmm..🤔

Ah, you are right, it should test against the metaclass. I still think the fix is wrong, TypeOf is already bound to those expressions.

You know, I'm not sure #5105 is a bug. The expression has no type, because its trying to search a method in a NoReturn. Or at least I don't think the behaviour is that terrible, I don't know when you'd want to do that anyway.

I'd rather not add more lines to the compiler which I don't think are the correct solution to the problem.

Well, it is a bug because it crashes the compiler but typeof(raise("").foo) doesn't crash the compiler, so this PR is "fixing" the wrong thing.

I think node.bind_to node.expressions is wrong, perhaps it has edge case. Correct code is simply that:

    def transform(node : TypeOf)
      node.type = @program.no_return.metaclass unless node.type?
      node
    end

In CleanupTransformer phase, it doesn't need to transform exp of typeof(exp) because exp is not evaluated on runtime, but to compile typeof(exp) correctly needs type for node.

OK, so I was wrong this whole time 😅

Your solution is more or less OK because the cleanup transformer might change some non-typed calls into NoReturn expressions. And when that happens the TypeOf node needs to rebind to the new expressions.

Basically this:

typeof(raise("").foo)

gets transformed to this:

typeof(untyped_expression!)

where untyped_expression! has a NoReturn type. But the typeof node is just bound to the previous expression so it must be bound to this new one, like what you are doing.

The problem is that in your code the TypeOf node is still bound to the old expressions, so now it gets bound to the old ones and new ones, instead of just the new ones.

So I was thinking of something like this:

diff --git a/src/compiler/crystal/semantic/cleanup_transformer.cr b/src/compiler/crystal/semantic/cleanup_transformer.cr
index 40693c55a..fe5cafae2 100644
--- a/src/compiler/crystal/semantic/cleanup_transformer.cr
+++ b/src/compiler/crystal/semantic/cleanup_transformer.cr
@@ -741,6 +741,17 @@ module Crystal
       node
     end
 
+    def transform(node : TypeOf)
+      node = super
+
+      unless node.type?
+        node.unbind_from node.dependencies
+        node.bind_to(node.expressions)
+      end
+
+      node
+    end
+
     @false_literal : BoolLiteral?
 
     def false_literal

That is, when the TypeOf ends up without a type (the only condition which can crash the compiler), we unbind from the current dependencies and bind to the new expressions.

What do you think?

(the specs you wrote are fine)

@asterite Perhaps you are right, but I'm not sure it works fine when there is untyped expression inside deep of typeof(exp) and this node affects result type.

An untyped expression has NoReturn as type, and when combined with other types this NoReturn just disappears. The only case is when all expressions are untyped and then type must change from "no type" to NoReturn, so I think it's fine.

In other cases, CleanUpTransformer never changes the type of something. It just changes from "no type" to NoReturn.

We can try this fix and if later we find another broken case we can fix that, but I think that won't happen.

@asterite Thank you! I fixed this as you suggested.