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

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

Closed
wants to merge 1 commit into from

Conversation

asterite
Copy link
Member

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
Copy link
Member Author

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
Copy link
Contributor

kostya commented Jul 14, 2016

what about T? i think it common case

@asterite
Copy link
Member Author

@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
Copy link
Member Author

@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
Copy link
Member

jhass commented Jul 14, 2016

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
Copy link
Member Author

@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
Copy link
Member

jhass commented Jul 14, 2016

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

@asterite
Copy link
Member Author

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
Copy link
Member

jhass commented Jul 15, 2016

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
Copy link

awesome!

@ujifgc
Copy link
Contributor

ujifgc commented Oct 30, 2016

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
Copy link
Member Author

@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
Copy link
Contributor

@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
Copy link
Contributor

kostya commented Jan 8, 2017

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
  1. when class redefine == method
class A
  def ==(b : B)
    @a == b.b
  end
end
class B
end

A.new == B.new
  1. when type part of union type:
A.new == (A.new || B.new)
  1. 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
Copy link
Contributor

kostya commented Jan 8, 2017

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
Copy link
Contributor

kostya commented Jan 11, 2017

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
Copy link
Contributor

luislavena commented May 24, 2017

@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
Copy link
Member Author

@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).

@asterite asterite deleted the feature/collections_restrictions branch April 27, 2018 17:52
@kostya
Copy link
Contributor

kostya commented Jul 21, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants