Add restrictions to collection methods. Fixes #1764. Fixes #988

[asterite]

Instead of rebasing #1851 I decided to do it again after discussing this better with @mverzilli and @juanedi

With this, all of these don't compile anymore:

[1, 2, 3].includes?(nil)
[1, 2, 3].includes?(1_i64)
[1, 2, 3].delete "foo"
[1, 2, 3] - [1, 'a']

{1 => 2}["foo"]
{1 => 2}["foo"]?
{1 => 2}.has_key?('foo")

In this way you get a compile error when trying to pass an invalid type to an operation. The reason behind this is that, for example, {1 => 2}["foo"] will always raise, so why would you want that in a program. Similarly, [1, 2].includes?("foo") is always going to be false, so that check is redundant.

This is a bit against generic programming, but the few cases I had to change/fix in the compiler, plus some other few cases I noticed needed to change in some shards makes me think this is a good change.

This is, of course, not backwards compatible.

[asterite]

Forgot to say: it would be nice if you could test this with some of your projects to see if it breaks a lot of code, and if the breakage is annoying and maybe we should keep the old behavior. So far I think that the annoyance rate is low enough to justify having better compile-time checks.

[kostya]

what about T? i think it common case

[asterite]

@kostya We thought about it, but I don't think it's worth adding this case. It can also lead to faster code. For example:

nilable_var = ...

# This needs to do a hash lookup, even is nilable_var is nil
value = hash[nilable_var]?

# This avoids a hash lookup if nilable_var is nil
value = nilable_var ? hash[nilable_var]? : nil

Basically, I don't think nil should be able to slip into methods just because it's convenient. It's better to be type-safe. The cases I had to change code was exactly because of nil, but in the end, even though the code is a bit longer, it's maybe more clear and definitely a bit faster.

Finally, a benchmark:

require "benchmark"

hash = {1 => 2, 3 => 4}
nilable = "foo".index('z')

Benchmark.ips do |x|
  x.report("no check") { hash[nilable]? }
  x.report("check") { nilable && hash[nilable]? }
end

Output:

no check 185.82M (± 9.09%)  1.90× slower
   check 352.46M (± 8.94%)       fastest

[asterite]

@jhass Diff needed for DeBot:

diff --git a/bot/src/plugins/admin.cr b/bot/src/plugins/admin.cr
index bb05ab1..00af8cd 100644
--- a/bot/src/plugins/admin.cr
+++ b/bot/src/plugins/admin.cr
@@ -13,14 +13,19 @@ class Admin
   end

   def superadmin?(user)
-    return false unless user.authname
-    config.superadmins.includes? user.authname
+    authname = user.authname
+    return false unless authname.is_a?(String)
+
+    config.superadmins.includes? authname
   end

   def admin?(user)
     return true if superadmin? user
-    return false unless user.authname
-    admins.includes? user.authname
+
+    authname = user.authname
+    return false unless authname.is_a?(String)
+
+    admins.includes? authname
   end

   #channel    =  ( "#" / "+" / ( "!" channelid ) / "&" ) chanstring
@@ -161,9 +166,9 @@ class Admin
     return unless superadmin? msg.sender

     authname = user(nick).authname
-    authname = {authname, nick}.find {|name| admins.includes? name }
+    authname = {authname, nick}.find { |name| name.is_a?(String) && admins.includes? name }

-    if authname
+    if authname.is_a?(String)
       admins.delete authname
       config.save(context.config)
       msg.reply "#{msg.sender.nick}: Removed #{nick} from admins."
diff --git a/framework/src/framework/message.cr b/framework/src/framework/message.cr
index e13fb8c..09925fd 100644
--- a/framework/src/framework/message.cr
+++ b/framework/src/framework/message.cr
@@ -16,7 +16,9 @@ module Framework

     def initialize(@context : Bot, @target : String, @message : String, @type : Symbol|String = "PRIVMSG")
       @type = @type.to_s.upcase
-      unless VALID_TYPES.includes? @type
+      type = @type.as(String)
+
+      unless VALID_TYPES.includes? type
         raise ArgumentError.new("Only valid types are #{VALID_TYPES.join(", ")}")
       end

@@ -32,7 +34,8 @@ module Framework
         @sender = @context.user
       end
       @type = message.type
-      @type = "PRIVMSG" unless VALID_TYPES.includes? @type
+      type = @type
+      @type = "PRIVMSG" unless type.is_a?(String) && VALID_TYPES.includes? type
     end

     def as_action

In general I see this change makes adding more nil checks and type casts in a few cases... I don't know if it's annoying enough to consider not implementing this change.

[jhass]

Mh, all of these are because they're nil unions. I think I wouldn't mind explicit checks if it weren't for the utterly verbose is_a?s, harming the little bit of duck typing we still having left in the language further. Maybe we can find an idiom like .try that's less verbose (and restricting) than .is_a??

[asterite]

@jhass In some cases the union was String | Bool | Nil, that's why I needed a cast. In other cases one could use try. user.authname, for example, is String | Bool | Nil. Is there are reason for that? If not, this change might actually be spotting a bug or a design issue.

[jhass]

The authname is the account name of an identified (authenticated with NickServ) user, nil is unknown, false is known to not be identified.

[asterite]

We might be needing something the language doesn't have. Maybe this should work:

# x : Int32 | String
[1, 2, 3].includes?(x)

The reason is that x has a type for which it might be true that the array includes it. If we pass a union where none of the type are in the array, then yes, we want a compile error.

On the other hand, maybe if you are doing includes?(x) and x has more types that that of the array, it might indicate a bug. It's probably all a matter of whether we think passing an incorrect type is a bug or not, but that can always give a false positive and some code becomes more tedious. It's almost the same as try and not_nil!.

[jhass]

I think that might be a great compromise, yeah. Perhaps something like a union fallback overload? A overload that gets called at runtime for elements in the union for which no other overload exists, but which is an error if it's the only match at compile time. Perhaps something like def includes?(other : !T).

[dsounded]

awesome!

[ujifgc]

Any ETA for this merge?

Had lots of Runtime debugging when porting ruby and javascript.

ary = "test".to_slice
puts ary.includes?('t') # => false
puts ary.includes?('t'.ord) # => true

[asterite]

@ujifgc It didn't turn out to be a good idea overall so we probably won't merge it (reasons are in this thread)

[luislavena]

@asterite it is possible to rebase this against master? I would love to check how hard it could affect some of our bigger codebases and have a more educated opinion on the usage.

Thank you.

[kostya]

I have idea how it can be implemented and cover all crazy cases. Just disallow compare different types with ==.

Allow compare with ==, only in 4 cases:

1. when class == class

A.new == A.new

2. when class redefine == method

class A
  def ==(b : B)
    @a == b.b
  end
end
class B
end

A.new == B.new

3. when type part of union type:

A.new == (A.new || B.new)

4. union type vs union type (compile when crossing between them is contain something):

(A.new || C.new) == (B.new || C.new)

All other compare cases disallowed to compile, because have no meaning to write if 1 == "bla", better to write if false then.

this allow catch so many programmer bugs like:

"test".to_slice.includes?('t') # compile error
{1 => 2, 3 => 4}["foo"] # compile error

also, some types like Int32 can define ==(other : Int64) method, then also would work: [1,2,3].includes?(1_i64)

[kostya]

recently i push #3847, but this was not real case

real case was this, i comparing in specs:

enum Enum1
  A
end

enum Enum2
  B
end

x = {} of Nil => Nil

x = x.merge({:a => Enum1::A})
x = x.merge({:b => Enum2::B})

y = {} of Nil => Nil
y = y.merge({:a => Enum1::A})
y = y.merge({:b => Enum2::B})

p x == y

this should work ok as 3 case from previous comment.

[kostya]

just made mistake again and debug it some time:

h = Hash(String, Int32).new
...
h.count { |v| v == 0 }

here i forgot add k in |k, v| while refactoring,
this code ok compiled, but comparison just always false.

[luislavena]

@asterite can you share the conclusion on why this is no good and deserves closing? (I wasn't able to test this with our bigger codebase 😢)

Thank you.

[asterite]

@luislavena Union types make this hard/impossible to get right. Maybe someone else will have a better idea on how to approach this problem - if it's considered a problem at all (I don't).

[kostya]

i run some experimental code which check == for type equality.
find two bugs, pretty sure its more:

• b here is byte, but compared with char
  https://github.com/crystal-lang/crystal/blob/master/src/http/server/handlers/static_file_handler.cr#L146
• compare int with enum
  https://github.com/crystal-lang/crystal/blob/master/src/flate/reader.cr#L98