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

[Interpeter] BUG: called llvm_struct_type for (Log::ShortColorFormat | TestFormat) (Exception) #12123

Closed
Vici37 opened this issue Jun 14, 2022 · 5 comments · Fixed by #12141
Closed
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:interpreter

Comments

@Vici37
Copy link

Vici37 commented Jun 14, 2022

I'm playing around with I3oris/ic on my largest project to see what edge cases I can run into. ic is a bit ahead of crystal's interpreter as it has the change from #12061 merged already, and this bug only appears after that one is fixed. Thankfully, this bug comes from one of my shards I've split out, so I have a simple reproducer below.

Stack trace:

BUG: called llvm_struct_type for (Log::ShortColorFormat | TestFormat) (Exception)                          
  from lib/ic/share/crystal-ic/src/compiler/crystal/codegen/llvm_typer.cr:365:7 in 'create_llvm_struct_type'    
  from lib/ic/share/crystal-ic/src/compiler/crystal/codegen/llvm_typer.cr:263:33 in 'llvm_struct_type'
  from lib/ic/share/crystal-ic/src/compiler/crystal/codegen/llvm_typer.cr:257:5 in 'llvm_struct_type'          
  from lib/ic/share/crystal-ic/src/compiler/crystal/codegen/codegen.cr:99:26 in 'instance_size_of'   
  from lib/ic/share/crystal-ic/src/compiler/crystal/interpreter/context.cr:324:11 in 'aligned_instance_sizeof_type'
  from lib/ic/share/crystal-ic/src/compiler/crystal/interpreter/compiler.cr:3210:5 in 'aligned_instance_sizeof_type'
  from lib/ic/share/crystal-ic/src/compiler/crystal/interpreter/primitives.cr:134:20 in 'visit_primitive'
  from lib/ic/share/crystal-ic/src/compiler/crystal/interpreter/primitives.cr:9:11 in 'visit_primitive'  
[snipped]

Reproducing:

> git clone https://github.com/Vici37/cr-color-logging
> cd cr-color-logging
> shards install
> ./bin/ic spec/color-logging_spec.cr

The shard is largely a reproduction of ShortFormat but with color configuration support added (I thought it looked a little bland).

@Vici37 Vici37 added the kind:bug A bug in the code. Does not apply to documentation, specs, etc. label Jun 14, 2022
@Vici37
Copy link
Author

Vici37 commented Jun 14, 2022

@I3oris new issue I ran into with ic

@I3oris
Copy link
Contributor

I3oris commented Jun 14, 2022

Reduced!:

require "log"

abstract struct Parent
  extend Log::Formatter

  def self.format(entry, io) : Nil
    new
  end
end

struct Child1 < Parent
end

struct Child2 < Parent
end

But it could surely be reduced more by going through log.

self.format is an abstract method defined by Log::Formater.

The bug doesn't happen if struct is replaced by class, It doesn't happen if format is defined in both Child classes instead of Parent.

Hope this help :)

@I3oris
Copy link
Contributor

I3oris commented Jun 14, 2022

After unfolding the log API I could reduce more:

module Formatter
end

abstract struct Parent
  extend Formatter

  def self.format
    new
  end
end

struct Child1 < Parent
end

struct Child2 < Parent
end

class Backend
  def initialize(@formatter : Formatter = Child1)
  end

  def write
    @formatter.format
  end
end

Backend.new.write

It seems when calling @formatter.format it tries to instantiate new on Parent because it includes Formatter, but Parent is abstract. However it work well with normal crystal run.

@I3oris
Copy link
Contributor

I3oris commented Jun 14, 2022

In fact, it's weird that is def initialize(@formatter : Formatter = Child1).

def initialize(@formatter : Formatter.class = Child1) sound more logical to me. but it doesn't compile:

Error: can't restrict Child1.class to Formatter.class

However that was the equivalent of what is in the log API. Maybe a bug here ?, or it's a property of extend keyword?

def initialize(@io = STDOUT, *, @formatter : Formatter = ShortFormat, dispatcher : Dispatcher::Spec = DispatchMode::Async)
super(dispatcher)
end

@asterite
Copy link
Member

I looked into it a bit. It's because we are missing handling virtual struct types in the interpreter. I'll probably fix it later this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:interpreter
Projects
None yet
4 participants