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

Compiler: don't say "undefined method" on abstract leaf types #8870

Merged
merged 2 commits into from
Mar 8, 2020

Conversation

asterite
Copy link
Member

@asterite asterite commented Mar 1, 2020

Fixes #6996

Consider this code:

abstract class Foo
end

foos = [] of Foo
foos.all?(&.valid?) # => true

The code above compiles fine in Crystal. The reason is that, because Foo is abstract, only its subclasses can actually be instantiated. But there are no subclasses! So there's no way to actually instantiate a Foo.

The compiler will then consider any call on such abstract types to be valid, but produce NoReturn... which makes sense: there's no way to reach that (but see the "Reaching that NoReturn" at the end).

The reason this also compiles is because Crystal tries to mimic Ruby. Let's consider this Ruby code:

class Foo
end

foos = [] of Foo
foos.all?(&:valid?) # => true

It works! Because we didn't pass any Foo, it's trivially true. Ruby doesn't even need to check that valid? method, and Crystal does the same thing.

Crystal is always "if you don't call it, we don't know". Which makes sense according to its prototypey and duck-typey nature.

But it didn't always work

If Foo had an abstract subclasses, the compiler used to complain:

abstract class Foo
end

abstract class Bar < Foo
end

foos = [] of Foo
foos.all?(&.valid?) # Error: undefined method 'valid?' for Foo+

The bug is that the compiler just checked whether Foo was abstract without subclasses. But the condition should also apply if Foo is abstract and all of its subclasses are in the same condition. The compiler calls this "abstract leaf".

It can also happen with generic types, even if they are concrete:

abstract class Foo
end

class Bar(T) < Foo
end

foos = [] of Foo
foos.all?(&.valid?) # => true

The above snippet didn't work, but with this PR it works. It works because Bar(T) has no generic instances. Foo was never actually instantiated.

And if you do instantiate Bar you'll get an error saying "undefined method 'valid?' for Bar(Int32) (compile-time type is Foo+)".

Why is this useful?

This is useful because one could define a hierarchy of abstract types, or generic types, and write code to work with that. Maybe a test creates instances. Maybe other tests don't because they expect third party shards to defined subclasses. All of these cases should work.

Reaching that NoReturn

Actually, there is:

abstract class Foo
end

ptr = Pointer(Foo).malloc(1)
ptr.value.valid?

The above compiles fine and raises an error on runtime:

can't execute `ptr.value.valid?` at /Users/asterite/Projects/crystal/foo.cr:5:3: `ptr.value.valid?` has no type (Exception)

But that's unsafe and it usually never happens in real code: the pointer is inside an array, there are bound checks, etc.

@yxhuvud
Copy link
Contributor

yxhuvud commented Mar 2, 2020

What I don't understand about the explanation is why malloc is allowed to allocate an abstract class. What does that even mean?

@asterite
Copy link
Member Author

asterite commented Mar 2, 2020

It's not allocating an abstract class. It's allocating space to put an abstract class.

@asterite
Copy link
Member Author

asterite commented Mar 2, 2020

Better said: space to put anything that inherits from that abstract class.

@yxhuvud
Copy link
Contributor

yxhuvud commented Mar 2, 2020

Ah ok, and it only works because it references have a fix size. Got it.

src/compiler/crystal/types.cr Outdated Show resolved Hide resolved
@bcardiff bcardiff added this to the 0.34.0 milestone Mar 2, 2020
@j8r
Copy link
Contributor

j8r commented Mar 2, 2020

I hoped it fixes #8853 too, but actually no.

Co-Authored-By: Brian J. Cardiff <bcardiff@manas.tech>
@bew
Copy link
Contributor

bew commented Mar 2, 2020

In my opinion, the first code should NOT work, unless you declare an abstract def for the method, so:

abstract class Foo
  # Without this is should fail to compile
  abstract def valid? : Bool
end

foos = [] of Foo
foos.all?(&.valid?) # => true

no?

@asterite
Copy link
Member Author

asterite commented Mar 2, 2020

@bew Ideally, maybe yes. Abstract methods are not required at all, they are just there to better document the code. The compiler doesn't use abstract methods at all except for checking that subclasses implement them. But it's a different topic.

@bew
Copy link
Contributor

bew commented Mar 2, 2020

Abstract methods are not required at all

But then, how foos.all?(&.valid?) is supposed to work type-wise if valid? isn't declared anywhere?

@asterite
Copy link
Member Author

asterite commented Mar 2, 2020

The compiler says: there are no concrete instances. It doesn't matter what this method is or how is this called, I'm going to type it as NoReturn.

@bew
Copy link
Contributor

bew commented Mar 2, 2020

@asterite I see thanks!

@RX14 RX14 merged commit 1334e76 into master Mar 8, 2020
This pull request was closed.
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.

Undefined method with empty array of abstract class T
8 participants