crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.34k stars 1.61k forks source link

`nil` check in `unless` doesn't give the expected result #1827

Closed Perelandric closed 8 years ago

Perelandric commented 8 years ago

I expect the first unless to produce "was nil" because str is nil, however it gives no output.

The second unless and the if behave as I would expect.

http://play.crystal-lang.org/#/r/kne

str :: (String | Nil)

unless s = str
  puts "was nil"    # this should run because `str` is `nil`
end

str = "foo"

unless s = str
  puts s    # this should not run because `str` is not `nil`
end

if s = str
  puts s    # this should run because `str` is `"foo"`
end

Crystal 0.9.0 (Fri Oct 23 20:47:03 UTC 2015)

jhass commented 8 years ago

This is not a bug, you shouldn't use str :: (String|Nil), it's unsafe and doesn't initialize the local with nil or anything it all, it doesn't even zero out the memory. You end up accessing garbage. Its only purpose is reserving stack memory to be passed to an external (C) library.

Perelandric commented 8 years ago

@jhass I see. That makes sense. I see that if I add str = nil below the first line, it behaves as expected. Thanks for the info.

asterite commented 8 years ago

@Perelandric @jhass We'll probably change the current syntax for something that makes it much more obvious that this is an unsafe feature. It's not the first time this happens and even though it's documented, since the same syntax is used for declaring instance vars this is pretty confusing.

Ideas for the new syntax? :-)

jhass commented 8 years ago

foo = Bar.stack_allocate?

asterite commented 8 years ago

Mmm... there's also this use case:

class Foo
  def initialize
    # Declare that we have a buffer, but don't zero the memory
    @buffer :: UInt8[1024]
  end
end

In this case it's not stack allocation, and the T might be a union and you can't do (T | U).stack_allocate

asterite commented 8 years ago

Maybe something like:

str :: unsafe String | Nil
@buffer :: unsafe UInt8[1024]

At least it'll be pretty obvious that it's unsafe :-)

If you don't specify unsafe inside a method, the compiler will tell you to do so and tell you "but note that if you do so the object won't be properly initialized, etc."

jhass commented 8 years ago

maybe reserve instead of unsafe?

Perelandric commented 8 years ago

Some syntax like this would be helpful I think. For some reason I've got it stuck in my head that those get a nil default.

Perelandric commented 8 years ago

Would it be crazy to suggest that those get a nil default? Then the rule would be that irrespective of the actual type, there's always an implicit | Nil. On the surface, it would seem to work into the rest of the system that handles nil already.

It forces you to handle it explicitly, which isn't as nice but it would make it safer anyway.

The implicit | Nil could also be avoided in some cases where a default value can be provided if Crystal allowed an inline assignment.

foo :: String   # (String | Nil)

bar :: String = "DEFAULT"   # String
ozra commented 8 years ago

How about the "standard word" alloca - but then the size would have to be known - if it propagates down the type instantiation, which I guess it doesn't though.

Couldn't above notation be changed so that a : Foo only types it (no unions possible by mistake). And then require explicit stack allocation if that's wanted, otherwise use the regular .new method for heap allocation.

foo : Foo
# as long as foo is not accessed before allocation no error is produced 
# from lacking assign to non (*|Nil)
foo = Foo.alloca  # allocates space for one Foo on the stack and calls initialize
foo = Foo.new     # allocates on heap via GC and calls initialize

# [ed] added below for completion:
foo2 : Foo = Foo.new  # fascist-typing it to avoid involuntary unions in a large 
                      # method by mistake
foo3 : Foo = Foo.alloca  # and the stack-allocation again, all on one row - staying
                         # "in style" with new-allocation

Also, the example with @buffer :: ... is very confusing, I think it should be a syntax error - that declaration should be directly in the root of the type imo. If there's no assign/allocation.

[ed2:] Perhaps there should be one way of alloca with initialize and one without, for when a C-func will init it...