Invalid Downcast Base+ <- Extended in non-captured block with restrictions

[toddsundsted]

Code like the following worked prior to 0.35:

require "digest/sha1"

Digest::SHA1.hexdigest do |digest|
  while rand > 0.5 # obviously, in production this is a more useful condition
    digest.update("")
  end
end

The error I get is:

$ crystal run test.cr
BUG: trying to downcast Digest::Base+ <- Digest::SHA1 (Exception)
  from raise<Exception>:NoReturn
  from raise<String>:NoReturn
  from Crystal::CodeGenVisitor#downcast_distinct<LLVM::Value, Crystal::Type+, Crystal::Type+>:NoReturn
  from Crystal::CodeGenVisitor#downcast<LLVM::Value, Crystal::Type+, Crystal::Type+, Bool>:LLVM::Value
  from Crystal::CodeGenVisitor#visit<Crystal::Var+>:(LLVM::Value | Nil)
  from Crystal::ASTNode+@Crystal::ASTNode#accept<Crystal::CodeGenVisitor>:Nil
  from Crystal::CodeGenVisitor#prepare_call_args_non_external<Crystal::Call, Crystal::Def+, Crystal::Type+>:Tuple(Array(LLVM::Value), Bool)
  from Crystal::CodeGenVisitor#visit<Crystal::Call>:Bool
  from Crystal::ASTNode+@Crystal::ASTNode#accept<Crystal::CodeGenVisitor>:Nil
  from Crystal::ASTNode+@Crystal::ASTNode#accept<Crystal::CodeGenVisitor>:Nil
  from Crystal::ASTNode+@Crystal::ASTNode#accept<Crystal::CodeGenVisitor>:Nil
  from Crystal::ASTNode+@Crystal::ASTNode#accept<Crystal::CodeGenVisitor>:Nil
  from Crystal::ASTNode+@Crystal::ASTNode#accept<Crystal::CodeGenVisitor>:Nil
  from Crystal::ASTNode+@Crystal::ASTNode#accept<Crystal::CodeGenVisitor>:Nil
  from Crystal::CodeGenVisitor#visit<Crystal::Call>:Bool
  from Crystal::ASTNode+@Crystal::ASTNode#accept<Crystal::CodeGenVisitor>:Nil
  from Crystal::CodeGenVisitor#visit<Crystal::Assign>:(Bool | Nil)
  from Crystal::ASTNode+@Crystal::ASTNode#accept<Crystal::CodeGenVisitor>:Nil
  from Crystal::ASTNode+@Crystal::ASTNode#accept<Crystal::CodeGenVisitor>:Nil
  from Crystal::CodeGenVisitor#visit<Crystal::Call>:Bool
  from Crystal::ASTNode+@Crystal::ASTNode#accept<Crystal::CodeGenVisitor>:Nil
  from Crystal::ASTNode+@Crystal::ASTNode#accept<Crystal::CodeGenVisitor>:Nil
  from Crystal::Compiler#codegen<Crystal::Program, Crystal::ASTNode+, Array(Crystal::Compiler::Source), String>:(Tuple(Array(Crystal::Compiler::CompilationUnit), Array(String)) | Nil)
  from Crystal::Compiler#compile<Array(Crystal::Compiler::Source), String>:Crystal::Compiler::Result
  from Crystal::Command#run_command<Bool>:Nil
  from Crystal::Command#run:(Bool | Crystal::Compiler::Result | Nil)
  from __crystal_main
  from main
Error: you've found a bug in the Crystal compiler. Please open an issue, including source code that will allow us to reproduce the bug: https://github.com/crystal-lang/crystal/issues

Link to playground reproducing the error: https://play.crystal-lang.org/#/r/99si

I am testing with 0.35 on OSX 10.14.6 (it works on 0.34 and earlier).

[toddsundsted]

investigating whether this is related to https://github.com/crystal-lang/crystal/pull/8426

[HankAnderson]

In Digest::Base, change the type restrictions that look like & : Digest::Base -> _ to be & : self -> _ to fix the problem. That's the actual type that's yielded. It's definitely a compiler bug, but change that to fix the regression.

In my opinion that PR refactor was a huge mistake. The core devs should pay more attention to what code they accept from external contributors. Such sensitive code like that shouldn't be handled to external contributors. There's inheritance there which is not needed at all. And macro inherited which is a big smell (I think @asterite said that too).

[toddsundsted]

thanks @HankAnderson with that guidance I created a test case that doesn't depend on Digest but recreates the error:

class Base
  def self.digest(& : Base -> _)
    yield new
  end

  def update
  end
end

class Extended < Base
end

Extended.digest do |digest|
  while rand > 0.5
    digest.update
  end
end

[HankAnderson]

It's even shorter:

def foo(& : Base ->)
  yield Extended.new
end

class Base
end

class Extended < Base
end

foo do |digest|
  while true
    digest
  end
end

[bcardiff]

Thanks @toddsundsted / @HankAnderson for reducing this. It seems that this is not a regression but something wrongly handled since the beginning of #8117 in 0.31.0. https://github.com/crystal-lang/crystal/issues/9483#issuecomment-644423912 can be reproduced in 0.31 ... 0.35.

I seems unintuitive that a downcast is been made instead of an upcast.

def m(& : T -> R)
  s : S
  yield s
end

Should type iff S < T, so as long as the yielded expressing can be upcast to the type argument of the block it should be sound. I didn't investigate further.

I will send a PR to workaround this issue in the context of Digest for now and include that in 0.35.1.

---

Regarding the refactors in digest, @HankAnderson I disagree that they should've been done only by maintainers. Reviews are part of the process and having a variety of authors encourage discovering and deciding best approaches and idioms within the language. Maybe reviewers overlooked something, I doubt the reviews were an unconscious approval.

This does not mean that the design can't be iterated and improved. I believe in partial/iterative progress. Other parts of the std-lib and compiler might require more trust, of course.

[toddsundsted]

thanks @bcardiff

[straight-shoota]

I fully support the latter remark of @bcardiff. Approving and merging code is really not about trusting the individual who wrote it. I personally know I can put trust in many contributors, both inside and outside the core team. But trusting the author doesn't mean their code is flawless. And for that it also doesn't make a difference whether you're on the core team. I'm sure every core member could name a few of their code changes that needed to be fixed later. That's just how things work. There's a review process to reduce the amount of mistakes getting merged. But it can't be eliminated. And instead of having fewer people (i.e. the core members) involved with writing and reviewing sensitive code, I'd like to encourage more people to do that. More minds on the same piece of code means less opportunities for bugs to hide.

[Sija]

@bcardiff This shouldn't be closed as the root cause of the OP's issue is still standing.

[bcardiff]

You're right.