Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Codegen: compile untyped typeof(exp) correctly #5109

Conversation

makenowjust
Copy link
Contributor

Fixed #5105

@@ -741,6 +741,19 @@ module Crystal
node
end

def transform(node : TypeOf)
# When the expression inside `typeof` node is untyped, that is
# `type(raise("").foo)` for example, then `node` is untyped also.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typeof

@makenowjust makenowjust force-pushed the fix/crystal/5105-compile-untyped-type-of-correctly branch from 5a160e3 to 744aabb Compare October 12, 2017 15:50
@asterite
Copy link
Member

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

@RX14
Copy link
Contributor

RX14 commented Oct 13, 2017

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

@asterite
Copy link
Member

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

@makenowjust
Copy link
Contributor Author

@asterite Updated. Okay?

@asterite
Copy link
Member

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

@@ -79,6 +79,12 @@ class Crystal::FixMissingTypes < Crystal::Visitor
end
end

def visit(node : TypeOf)
node.expressions.each &.accept self
node.type = @program.no_return.metaclass unless node.type?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should just be @program.no_return. In the spec too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think correct type for typeof(raise("").foo) is NoReturn:class because typeof(raise("")) is typed as NoReturn:class.

@makenowjust
Copy link
Contributor Author

@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.

@asterite
Copy link
Member

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.

@makenowjust
Copy link
Contributor Author

makenowjust commented Oct 16, 2017

@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
#                     ^~~
#

@lbguilherme
Copy link
Contributor

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.

@makenowjust
Copy link
Contributor Author

makenowjust commented Oct 16, 2017

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..🤔

@asterite
Copy link
Member

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

@asterite
Copy link
Member

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.

@asterite
Copy link
Member

asterite commented Oct 16, 2017

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.

@makenowjust
Copy link
Contributor Author

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.

@asterite
Copy link
Member

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)

@makenowjust
Copy link
Contributor Author

@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.

@asterite
Copy link
Member

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.

@makenowjust
Copy link
Contributor Author

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

@RX14 RX14 merged commit 4721e6b into crystal-lang:master Oct 16, 2017
@RX14 RX14 added this to the Next milestone Oct 16, 2017
@makenowjust makenowjust deleted the fix/crystal/5105-compile-untyped-type-of-correctly branch October 16, 2017 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants