Crystal: The result of comparing signed and unsigned variables of the same size is inconsistent

[ssvb]

Let's take a look at the following test program:

a = 4294967295_u32
b = 0_i32

puts "a = #{a}"
puts "b = #{b}"

if b > a
  puts "(b > a) is true"
else
  puts "(b > a) is false"
end

if a < b
  puts "(a < b) is true"
else
  puts "(a < b) is false"
end

The resulting output is

a = 4294967295
b = 0
(b > a) is true
(a < b) is false

The statements "(b > a) is true" and "(a < b) is false" are obviously contradicting each other.

C/C++ compilers usually issue a warning about mixed signed/unsigned comparisons even though the language standard defines deterministic (albeit somewhat convoluted) conversion rules:
https://www.securecoding.cert.org/confluence/display/cplusplus/INT02-CPP.+Understand+integer+conversion+rules

[asterite]

I think we can disallow comparisons between different signs.

On a side note, unsigned integers are only in the language to be able to easily bind to C, nothing more. One usually uses Int32 or Int64.

[ssvb]

I think we can disallow comparisons between different signs.

This is a reasonable solution, both Rust and Go disallow them too:

test.rs:5:12: 5:13 error: mismatched types:
 expected `i32`,
    found `u32`
(expected i32,
    found u32) [E0308]
test.rs:5     if b > a {

./test.go:12: invalid operation: b > a (mismatched types int32 and uint32)

[ssvb]

On a side note, it is probably possible to actually implement such comparisons in a mathematically correct way (somewhat inspired by #559) if we don't mind having two comparisons happening under the hood. First it is possible to check whether the signed operand is negative (if it is negative then we already know that it is smaller than the unsigned operand). And if the signed operand is non-negative, then just compare both operands as unsigned numbers.

Always rejecting signed/unsigned comparisons may potentially make the compiler too anal about certain code constructs, for example:

  a = 0_u32;
  while a < 100
    puts a
    a += 1
  end

The integer literal is treated as a signed integer and might make the compiler complain here. However if the mathematically correct two-step comparison is implemented, then the check whether the signed operand is negative (the integer literal 100) should be optimized out by the compiler because it knows the exact value at the compile time.

Still just rejecting such comparisons is probably much easier to implement and guaranteed not to cause unexpected problems. Other than annoying the abusers of unsigned integer types, who would need to litter their code with explicit casts :-)

[stugol]

Has this STILL not been fixed? It's kinda important that numbers work correctly in a programming language.

[omninonsense]

You shouldn't compare signed and unsigned numbers without first converting them to the same type, though. The compiler should only issue a warning (as does C), IMO.

[refi64]

To me, the main issue is that, because of type inference, you may not _know_ what's signed and unsigned.

OTOH, this probably isn't that high-priority because most people likely don't deal with integers that big on a normal basis...

[stugol]

....excuse me? Basic mathematical operations being totally up the swanny isn't important you say?

I have been working, on and off, on an rsync-style backup program written in Crystal. If it's going to fall over every time it encounters a file larger than 2GB, I'd say that was a pretty high priority! And don't give me that "you shouldn't compare signed and unsigned numbers" nonsense - of course you should! This isn't assembly! I'm coding in a high-level language: the internal representation of an integer should be none of my concern.

Ten months this issue has remained unfixed. Ten months! When it's such a fundamental problem, and when a solution is so simple to implement.

I think it's time I found myself another language. Rust perhaps, or Felix. Because you guys obviously aren't taking things seriously.

[waj]

I wont spend any minute of my life trying to answer your rants. Please, do whatever makes you happy.

Also, please stop trying to make us feel bad about what we do, because you're not suceeding at all.

[stugol]

Yes, obviously my entire purpose here is to make you feel bad. It's not at all that I want a programming language that works. Definitely not.

[omninonsense]

@stugol Well file sizes are unsigned integers, so you should've used an unsigned integer to begin with. I understand the frustration and I'm sorry you feel that way.

I don't think Crystal is as high level as you imagined it to be. It's a few steps up from C, but it's also a few steps down from Ruby.

Checking the type during each comparison would be a performance waste.I was wrong, never mind this.

I don't think Implementing it is not the problem, but coming up with a good method is. If you see the solution, why don't you share it?

[refi64]

@stugol Calm down a bit! This isn't war, you know. ;)

Remember: Crystal is still technically in alpha, so things aren't always going to work correctly. Now, if Crystal were already at 1.0, then this would _definitely_ be really bad. But it's not even in beta, so there will be things that just won't work 100% correctly yet.

[refi64]

Also, would anybody be open to a PR for this? I think that this can be fixed via something like:

def gt(lhs : Int32, rhs : UInt32)
  return lhs >= 0 &&
         lhs.to_u32 > rhs
end

[bcardiff]

@omninonsense Crystal compiler is warning free. It's a bit more terse. We try to avoid compiling things that may not run, obvious not 100% accurate but this scenario is like NullPointerException IMO.

With a the appropriate overloads of methods and mathematical reasonable bound checks it could be implemented without affecting runtime performance of operators between both types. A

At least for comparison I think it's a nice to have.

But for operations between ints you will lose track of the type pretty soon. I bet unions that won't make a lot of sense some times will appear.

So, I vote yes for comparison across types.
And not compile, at least for now for other operations that mixes numerical types.

[omninonsense]

@bcardiff Good point about the fact that it could be free with overloads, it didn't cross my mind earlier.

[stugol]

def gt(lhs : Int32, rhs : UInt32)

So...if I write a gt function, it'll be called whenever I < ? I did not know this. I tried writing a def <( ... ) to fix the problem, but it did nothing at all. It did compile, however, which is strange. Why have def gt and not def <?

Is this keyword/operator duality documented somewhere?

@bcardiff I fail to see how mathematical operators silently lying to me is anything remotely similar to a NullPointerException. In the latter case, it is obvious that a problem has occurred.

@omninonsense I don't have to. @ssvb already did.

And I would be happy to contribute, but if I try to compile Crystal using a version of Crystal I have already compiled, _it will not compile!_ I raised an issue about this days ago, and it has been ignored. So how am I supposed to contribute?

I've written a reduce? function already, and would have issued a PR, but I can't compile it.

[stugol]

You shouldn't compare signed and unsigned numbers without first converting them to the same type, though.

That makes no sense whatsoever. If the signed integer contains a negative number, converting it to unsigned would be wrong. And similarly for the other way.

[stugol]

Well file sizes are unsigned integers, so you should've used an unsigned integer to begin with.

Further up, @asterite said we shouldn't use them. So which is it?

[refi64]

@stugol No, there's no operator duality. That was just an example...

[stugol]

@kirbyfan64 Well, if I implement def <(...) on a built-in int type, the compiler completely ignores it. Why is this?

[asterite]

I'll just comment, to prevent a flood of messages trying to figure out why that doesn't work, that some primitive methods, like math for numbers, are predefined with a type, so redefining them doesn't work right now.

[daliborfilus]

This issue stil exists:

$ crystal --version
Crystal 0.26.1 (2018-08-27)

LLVM: 6.0.1
Default target: x86_64-apple-macosx

$ cat spec/signed_spec.cr
require "./spec_helper"
require "crc32"

describe Gen do
  it "excuse me?!" do
    -616_928_420_i32.should eq(3_678_038_876_u32)
  end
end

$ crystal spec
Finished in 0.01 milliseconds
1 examples, 0 failures, 0 errors

The numbers are the same (bite wise), but the eq operator should at least throw a warning - or the code shouldn't compile at all.

[ysbaddaden]

What about the initial solution, to drop support for comparing mixed signed/unsigned integers?

• The comparison is complex, and error prone —as shown in this issue.
• Unsigned integers are only meant for special cases, such external library interactions or implementing algorithms (e.g. crypto, hash, binary protocols). They're not supposed to be used on a daily basis.

I think nobody will miss really this (manually cast if needed). It may even prove useful and help fix a few bugs.

[asterite]

We already have custom logic to compare signed and unsigned integers with <, <=, > and >=. I think it's just about doing the same with ==, basically: if one of them is unsigned and the other not, and it's negative, then == is always false and != is always true. I can probably do this, it's super simple.

[daliborfilus]

I agree that unsigned integers can be used scarcily, but the stdlib itself uses them - I found this issue by using CRC32 module (https://crystal-lang.org/api/0.26.1/CRC32.html) lwhich returns UInt32.

I wanted to compare it to signed int 32 returned by crc32 from Java, and I expected the test to fail, but it didn't, that's why I wrote that spec above.

I wanted to convert it to signed before comparison anyway, but the test should've failed.

I agree that the types are different and the comparison should be illegal.
Fixing "==" operator to return false in negative cases is an option too, but it may hide bigger problem somewhere else in the program...

[asterite]

We should probably return Int32 from CRC32.

[ssvb]

Or maybe in the case of CRC32 it's better to do it in a type safe object oriented way and introduce a new class for the CRC32 checksum? Which can be explicitly converted to signed or unsigned numbers via ".to_i32" and ".to_u32" methods depending on what the user wants.