Crystal: No type error when deleting from an array an invalid type

Created on 16 Jul 2015  路  7Comments  路  Source: crystal-lang/crystal

Example:

ar = [] of Int32
ar.delete "abc"

This has caused bugs for me before.

accepted draft stdlib

Most helpful comment

I think it makes sense to put a type restriction there. The same happens with Hash#[], which doesn't have a restriction but maybe it should.

All 7 comments

Real-life example:

Consider these two functions:

      # Mark a register as no longer used.
      def freereg(reg)
        @usedregs.delete reg
      end

      # Free a register if it's a register item.
      def ofree(maybereg)
        self.freereg maybereg.reg if maybereg.is_a? Parser::RegItem
      end

I had originally written the second-to-last line as:

self.freereg maybereg if maybereg.is_a? Parser::RegItem

@usedregs is supposed to be of the type Register, which is completely independent from Parser::RegItem. Because I was passing the wrong type to freereg, the register was never freed. This caused that to lurk as a bug until my compiler crashed because it ran out of registers.

Possible fix: change line 418 of array.cr to:

def delete(obj : T)

This is interesting :-)

In Java for example removing an element is not restricted to a type. This is because you delete elements by equality. So, this works in Ruby (and in Crystal):

a = [1, 2, 3]
a.delete 1.0
a #=> [2, 3]

So I'm not sure what we should do here. This is an excellent opportunity to discuss it :-)

@asterite Well, Ruby is duck-typed, and Java is pretty stupid. ;)

I'm just saying this because I don't think anyone has ever done that intentionally, as it doesn't really serve a purpose. What would the point of it be? a could never hold floats, anyway.

Just my three cents. :)

I think I side with @kirbyfan64 here, for the sole purpose of catching type errors earlier

I think it makes sense to put a type restriction there. The same happens with Hash#[], which doesn't have a restriction but maybe it should.

Was this page helpful?
0 / 5 - 0 ratings