Collision between module and abstract class breaks the code generator, with null reference in LLVM

[BrucePerens]

Crystal 1.2.2 [6529d725a] (2021-11-10)

LLVM: 10.0.0
Default target: x86_64-unknown-linux-gnu

This program:

module M
  @a = 1
end

abstract class A
  @a = 1
end

class B < A
  include M
  @b = 1
end

B.new

Results in this error inside of LLVM:

Invalid memory access (signal 11) at address 0x0
[0x7faff3351016] ???
[0x7faff3350fe3] ???
[0x7faff43c4a14] ???

With the bleeding-edge compiler and LLVM 13:

Crystal 1.3.0-dev [ee9996728] (2021-12-22)

LLVM: 13.0.1
Default target: x86_64-unknown-linux-gnu

I get a better report:

crystal: .../llvm-project/llvm/include/llvm/IR/Instructions.h:921: llvm::Type* llvm::checkGEPType(llvm::Type*): Assertion `Ty && "Invalid GetElementPtrInst indices for type!"' failed.
Aborted

[HertzDevil]

Related: #10979

[BrucePerens]

@beta-ziliani Please prioritize, this has been in the compiler for at least 6 months, according to #10979, and results in no comprehensible error message, so is really difficult for the user to isolate.

[robacarp]

@BrucePerens you saved me a lot of time binary searching my own source code for this. Thank you.

[BrucePerens]

@BrucePerens you saved me a lot of time binary searching my own source code for this. Thank you.

@robocarp You're welcome. It did indeed take quite a while. And @beta-ziliani, here's someone else who hit that bug.

[straight-shoota]

This appears to be pretty much the same issue as #10979, it appears when merging modules or modules and classes.

The reproduction in the OP also works with a concrete class, so abstract is not a condition for this error.

It's clear that this error should be fixed, but I'm not entirely sure about what would be the expected behaviour. Is it inevitably an error? Or can the compiler accept ivar redefinitions from different hierarchies? In the example, the ivars' types are identical, so they could be trivially unified. This works in linear hierarchies as well. The only trouble is figuring out precedence of default values, but I presume that should just follow the overload ordering of methods (i.e. M overrides A).

Incompatible types must certainly be an error, like in linear hierarchies.

Does that sound good?
Or maybe we should just start with producing a proper error message and improve things afterwards?

[asterite]

I think the example in this issue should compile fine. The main problem is figuring out how to fix this.

[robacarp]

A readable error would certainly be helpful in moving forward, but Bruce's example is certainly not something that seems broken to me. Resolving default values by method overloading paradigms makes sense at first glance.

[straight-shoota]

It appears the case of different types is already handled correctly. So it seems to be only a matter of unifying the instance variable when the types are compatible.

module M
  @x = 'a'
end

abstract class Parent
  @x = 1 # Error: instance variable '@x' of Parent must be Int32, not (Char | Int32)
end

class Child < Parent
  include M
end

The error location isn't accurate, it should probably be on Child, not Parent.

[asterite]

I looked into this briefly the last week to see if I could quickly fix it, but I failed.

To understand this problem better, you have to know that:

• instance variables of modules are just used to compute what instance variables classes and structs will have
• in a hierarchy, an instance variable belongs to the top-most type that defines it

So for instance if we have:

class A 
  @a = 1
end

class B < A
  @b = 1
end

We have that A holds @a : Int32 and B holds @b : Int32. Also B holds @a : Int32 through A, but that information is kept in A, not in B.

Let's say we have this:

```crystal
class A 
  @a = 1
end

class B < A
  @a = 3
  @b = 1
end

In this case there's also @a : Int32, but the end result is the same as before: @a : Int32 belongs to A, B knows nothing about it directly (it knows about the initializer, though)

To be able to do this the compiler will analyze type hierarchies starting from root types and going downwards.

This... kind of falls apart with modules!

What Crystal does right now is to analyze modules first, before classes and structs. If a module mentions an instance variable, that instance variable is propagated to inherited types.

So in a code like this:

module Moo
  @a = 1
end

class Foo
  @b = 2
end

• We first process Moo, and then we know that Foo needs to have @a : Int32
• Then we process Foo and learn that it also has @b : Int32

The end result is that Foo will have:

• @a : Int32
• @b : Int32

So far, so good.

But with this example:

module M
  @a = 1
end

abstract class A
  @a = 1
end

class B < A
  include M
  @b = 1
end

B.new

1. We first process M and we learn that B needs to have @a : Int32
2. We then process A and learn that it needs to have @a : Int32
3. We then process B and learn that it needs to have @b : Int32

Do you see the problem? The definition of @a : Int32 now lives in both A and B! This is the bug that is being triggered in the compiler.

How to fix this? Hard! I have no idea, hehe...

One idea is, when in point 2 we learn that A has @a : Int32, check if any subtypes also define that instance variable. In this case there's B! So the compiler could "steal" that variable from B and just leave it in A.

Well, maybe that could work but I didn't have time to try it... (not I'll have time)

Another alternative could be to first process type hierarchies and then modules. If we do that, then:

1. We first process A and learn that it has @a : Int32
2. We then process B and learn that it has @b : Int32
3. We then process M, which is included by B, and learn that it needs to have @a : Int32 but it already exists in a parent type so we do nothing

This I tried, but type initialization breaks. It seems the logic to determine instance variables is tied with determining which ones are initialized in initialize, and here we also need to take modules into account.

So... maybe now you understand why this issue has been sitting here for a long time. It's hard! The core team can prioritize it but that doesn't mean it will get fixed soon. It's like asking to prioritize solving the halting problem :-)

[BrucePerens]

I sympathize with the desire to have the compiler actually process this construct correctly. But since this is a difficult thing to do, could we have it detect the issue and emit a message, in the short term?

[straight-shoota]

Thanks @asterite, this was very insightful. I took the liberty to export your explanations to the wiki (https://github.com/crystal-lang/crystal/wiki/Compiler-Internals:-Instance-Variables) to make it discoverable.