Raise if passing null Pointer to String

[jgaskins]

This is probably preferable to a SEGV.

Source discussion

I’ll add some specs for it, but I just wrote this in the browser on an iPad for the moment and while I’m thinking about it.

Also, I don’t know if it makes sense to create a new exception class for this particular bug, because Exception feels too broad.

[jhass]

If we want this it probably should be an ArgumentError.

"Cannot generate" feels weird, this isn't really generating anything.

[jhass]

Let's have some simple specs for this

[jhass]

I'm fine with this behavior, but it might warrant some discussion.

[Sija]

@jhass Why? This PR doesn't change the way it works right now.

[jhass]

Previously null pointers were just not considered by the implementation. That it doesn't change behavior for this case is more of an implementation detail than design. This considers how to deal with null pointers from a design perspective, so the consideration to change this is within scope in my opinion.

[jgaskins]

I'm surprised there were no specs about pointers with 0 length because it seemed to be an intentional choice in the code.

[bcardiff]

I think I would prefer String.new(Pointer(UInt8).null, 0) to raise. As raised by @jhass null pointers here were probably not considered before. It think it will also be more consistent with the behavior of String.new(Pointer(UInt8).null).

The optimization that lead to the bytesize == 0 check is about reusing the empty string literal embeded in the binary.

@jgaskins I know it's been a while, let me know if you prefer to hand of these changes.

[jgaskins]

@bcardiff I'm happy to turn it over to someone that understands the implications better than I do. I mainly wanted to see if there was a better way of handling char *s = 0 from a C function since someone I was chatting with about Crystal ran into it.

[bcardiff]

@jhass @asterite Do you agree then to make String.new(Pointer(UInt8).null, 0) raise? If so, I can finish this up.

Thanks @jgaskins . The intention was to fail in a more controlled way in case of char *s = 0, right? Is not that that person was expecting s to be an empty string, right?

[jgaskins]

Whichever makes the most sense. I think either case is better than a segfault. I originally had the same perspective @asterite did in this comment, but your response to it makes a lot of sense. And since any time we're dealing with pointers, we're generally also dealing with C code, I feel like making the distinction between an empty string and a null pointer seems like a reasonable requirement.

[jhass]

I feel okay either way. We can see if somebody runs into trouble with the changed behavior. It seems reasonable to me that something would return a null pointer and a zero length in an error or "not found" case, but have no example at hand to back it up.

[jgaskins]

The hilarious thing about the build is that I seem to have somehow created a CircleCI build on my own fork of the repo, which doesn't have macOS builds enabled, so the macOS build failed. I have no idea how to cancel it. 😂

[bcardiff]

I think I would prefer String.new(Pointer(UInt8).null, 0) to raise. As raised by @jhass null pointers here were probably not considered before. It think it will also be more consistent with the behavior of String.new(Pointer(UInt8).null).

The optimization that lead to the bytesize == 0 check is about reusing the empty string literal embeded in the binary.

@jgaskins I know it's been a while, let me know if you prefer to hand of these changes.

[asterite]

I think raising on null pointer with length zero might be a breaking change and actually annoying. For example you ask a string from a C library. It turns out it's the empty string. The only way one can represent that in C is with a null pointer. Maybe it then returns the length of the string, which is zero. You pass those to String.new and get an exception. And worse, you have no easy way to fix it. You have to special case it.

There's nothing dangerous about passing a null pointer with a size zero. I think we should keep that behavior.

[bcardiff]

The only way one can represent that in C is with a null pointer

I'm confused. Usually functions with char* assume valid pointers. Behavior on null is undefined.

Do you recall C functions that return null as an empty string? Why couldn't they return a pointer to a \0 char?

[asterite]

Oh, I think you are right. For the empty string it will be a pointer to the null character, not a null pointer.