Undefined constant Digest::CRC32

Help & Support
1

I have this stumpy_png shard in my project that I’m trying to update for Crystal 0.36.1, but I keep running in to this issue which I originally reported to the shard.

In src/stumpy_png/crc_io.cr:18:12

 18 | @crc = Digest::CRC32.update(slice, @crc)
             ^------------
Error: undefined constant Digest::CRC32

I tried to break out the issue outside of my project to narrow down what was causing it, and I noticed something weird. Take this example:

require "digest" # <-- if I remove this line, it'll compile
require "digest/crc32"

class CrcIO < IO # <-- or don't inherit from IO
  getter crc : UInt32
  property size
  
  def initialize
    @crc = 0_u32
    @size = 0
  end
  
  def read(slice : Bytes)
    0
  end

  def write(slice : Bytes) : Nil
    @crc = Digest::CRC32.update(slice, @crc)
    @size += slice.size
  end
  
  def reset
    @crc = 0_u32
  end
end

It seems to be some combination of requiring digest, and inheriting from IO that causes the Digest::CRC32 to randomly disappear…

Now, in the shard, it never calls require "digest" which means that the shard on its own compiles fine and specs run and pass. My guess is my Lucky app must have a require somewhere that’s causing this issue, but I haven’t found where or what yet. The other strange thing is, if I replace that require "digest" with a require "digest/digest", it’ll also work. Unfortunately, the patch doesn’t seem to work in my app directly. As in, I can’t just throw a random require "digest/digest" at the top of my app and have it work.

Anyone have any ideas?

2

This is probably the issue: crystal/io_digest.cr at master · crystal-lang/crystal · GitHub

I really think we should rename that to DigestIO. Then also MemoryIO and so on. I think it was a very bad decision to try to namespace everything…

1 Like
3

Yup is probably that.

Workaround:

  def write(slice : Bytes) : Nil
---    @crc = Digest::CRC32.update(slice, @crc)
+++    @crc = ::Digest::CRC32.update(slice, @crc)
    @size += slice.size
  end
1 Like
4

I find it actually a bit surprising is the namespace lookup from an inherited scope which takes precedence over literal scope. Basically this:

class Bar
  BAR = "::Bar"
end

class Baz
  class Bar
    BAR = "Baz::Bar"
  end
end

class Foo < Baz
  Bar::BAR # => "Baz::Bar"
end

It works like this in Ruby. And I suppose you would expect that mostly for constants for easy access to constant values defined in an ancestor.
But in the context of namespaces, it feels somewhat unexpected to me. Foo isn’t in the Baz namespace, it’s in top level namespace and thus I’d expect Bar to mean ::Bar not Baz::Bar.

There’s probably no way to change this without losing consistency. This is an effect of types also being namespaces, so I guess we’ll have to accept it.

1 Like
5

Yeah, the lookup works exactly the same as in Ruby. Maybe it’s a bad thing :-)

The problem with it is that it allows type-hijacking. For example, I define a library:

class Foo
  def self.foo
    Bar.bar
  end
end

class Bar
  def self.bar
    puts "Hello!"
  end
end

Foo.foo # "Hello"

Then someone comes and does this:

class Foo
  class Bar
    def self.bar
      puts "Bye!"
    end
  end
end

Then the Bar I was referring to in Foo is no longer the same type, and I get “Bye!” as an output.

This is not that bad if you don’t use the same names inside namespaces that are also part of an outer namespace.

Again, I was against naming IO::Memory like that (MemoryIO is better: its name is more readable and clear, there’s less typing, and there’s less chance of a name clash). Same with StringBuilder, DigestIO, etc. It really makes no sense that it’s called IO::Digest… what is it, a digest? No! It’s a “digest IO”, so it should be called DigestIO. What’s IO::Memory? Is it a memory? No! Well, I’ll stop my rant now…

By the way, the way C# names things is exactly the same as what I’m saying here. They have MemoryStream, they don’t have Stream.Memory.

4 Likes
6

Thanks for all the info! That at least lets me move past my issue so I can continue the upgrades.

7

I suppose in a more positive way this could also be considered a feature for injecting mock implementations for tests. :laughing:

MemoryIO is certainly better as a name than IO::Memory. And there are similar cases like *::Builder, *::Reader, *::Writer, *::Parser.

But the problem is, if we rename all of them, there’s gonna be a whole lot more types in the top level namespace. We’d have DelimitedIO, StapledIO, FileDescriptorIO, MemoryIO, HeydumpIO, SizedIO, MultiWriterIO just from the IO namespace. Polluting the namespace may or may not be a huge issue. For the API docs it would definitely mean to lose context.
Before even considering to change anything about this, we would need to find a really better alternative. Otherwise it’s not worth any migration cost.

2 Likes