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

Typecheck default values if unusued #2636

Closed
pkorotkov opened this issue May 23, 2016 · 6 comments
Closed

Typecheck default values if unusued #2636

pkorotkov opened this issue May 23, 2016 · 6 comments

Comments

@pkorotkov
Copy link

pkorotkov commented May 23, 2016

This code triggers an expected error

class A(T)
  getter values

  def initialize(@values : Array(T) = Int)
  end
end

a = A(String).new

p typeof(a.values)

#link 1

but the following snippet compiles and runs just fine contrary to my expectations

class A(T)
  getter values

  def initialize(@values : Array(T) = Int)
  end
end

a = A.new(values = ["One", "Two"])

p typeof(a.values)

#link 2

@jhass
Copy link
Member

jhass commented May 23, 2016

Reduced

class A
  def initialize(@value : Int32 = true)
  end
end

a = A.new

vs

class A
  def initialize(@value : Int32 = true)
  end
end

a = A.new(1)

@jhass jhass changed the title No type inference error in a generic class Typecheck default values if unusued May 23, 2016
@asterite
Copy link
Member

Not sure this is a bug. Right now you can do:

# You can only pass a String, but the default value is nil
def foo(value : String = nil)
end

foo # OK
foo "hello" # OK
foo nil # Error

In that way you can restrict an argument to a type but use a default value of another type.

@jhass
Copy link
Member

jhass commented May 23, 2016

That's just even more inconsistent, isn't it? Whether it errors depending on the type of variable that's restricted. We decided to conflate restriction and declaration for instance variables there, we should keep the rest of the language consistent to it.

Also I don't really see a real usecase behind your example tbh.

@asterite
Copy link
Member

asterite commented May 23, 2016

I was thinking about having nil there to know that a value that's not possible to pass was received in the method, so you know no argument has been passed. This could also be useful if you use the null object pattern. But yeah, it's probably inconsistent. I'll try to fix this, though the error will happen if you invoke the method without arguments, so the second snippet in the first comment will still compile fine (so the end result will be more or less the one we have right now)

@bcardiff
Copy link
Member

IMO it is a bug. It will force to implementor some T? for nillable arguments but I think is the right thing to do.

Maybe there will be a tricky part when the method involves type variables. I was trying some snippets but might reach a separate issue here:

https://play.crystal-lang.org/#/r/zbd

def foo(o : K, h : Hash(K, V) = { "foo" => "bar" })
  h.keys
end

pp foo(7, {1 => 2}) # => [1]
pp foo("sdf") # => ["foo"]
pp foo(8) # => ["foo"] 

@asterite
Copy link
Member

asterite commented Jan 6, 2017

Part of this was fixed in #3834

The original title of this issue is "Typecheck default values if unusued", which basically means that this should give a compile error:

def foo(x : Int32 = "hello")
end

That is, it should give a compile error even if we don't invoke foo. I'm not sure about that. Right now we only type-check things on invocation. For example this works fine in Ruby:

def foo(x = unknown)
end

foo(1)

But if we don't pass an argument we get an error. Well, it's the same in Crystal here, and I think that's OK (having to type check all methods even if they are not used will slow down compilation, and I'm not sure it's even possible in all cases)

@asterite asterite closed this as completed Jan 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants