unsigned conversion regression

[mratsim]

The following used to give 44 on 0.20.2.

It now raises an error range error on 1.0.

let x = 300'u
echo byte(x)

This broke our upcoming networking code: vacp2p/nim-libp2p@541f0f2. In general, I don't think people expect range checking on unsigned integers, especially on converting to raw byte.

This may have a large impact on low-level code for example: graphics, crypto, networking, emulation ...

[kraptor]

For low-level code, what I always do is using proper casting to avoid this:

let x = 300'u
echo cast[byte](x)

[Araq]

Here is what the spec says about it:

`Automatic type conversion`:idx: is performed in expressions where different
kinds of integer types are used: the smaller type is converted to the larger.

A `narrowing type conversion`:idx: converts a larger to a smaller type (for
example ``int32 -> int16``. A `widening type conversion`:idx: converts a
smaller type to a larger type (for example ``int16 -> int32``). In Nim only
widening type conversions are *implicit*:

Type conversions
----------------
Syntactically a `type conversion` is like a procedure call, but a
type name replaces the procedure name. A type conversion is always
safe in the sense that a failure to convert a type to another
results in an exception (if it cannot be determined statically).

So it now behaves as the spec requires and previously it didn't. But I agree the wording is not explicit about unsigned types and worse there was no changelog entry for this change.

[Araq]

This may have a large impact on low-level code for example: graphics, crypto, networking, emulation ...

We will add a changelog entry and we might add a switch to turn it off again, but looking at it without the history, it seems to be an improvement, use bit masking or cast to convert to bytes. Nim got more consistent in its behaviour.

[dryajov]

I generally agree with the behavior - it is safer, but the problem is that it fails at runtime, it should be caught at compile time and warn at the very least, tho failing compilation is much more preferable.

[Araq]

Alright, good. Let's make this create a new warning.

[dryajov]

Dope!

[krux02]

@Araq what kind of warning do you have in mind?

[Araq]

Warning: This type conversion now produces a runtime check that can raise

[dryajov]

How about:

Warning: narrowing type conversions are [deprecated/not allowed] 🚲 🏠

[krux02]

What if this runtime check is exactly what I need?

[dryajov]

I think this isn't for checks, its for assignments specifically, thats where I would expect it to warn.

let myByte: byte = 300 should warn

[krux02]

that one isn't allowed, it is checked at compile time.

[dryajov]

You're right, that one is caught, this one isn't (https://play.nim-lang.org/#ix=20lj):

let val: uint = 300
let myByte: byte = byte(val)

[Araq]

Then you turn off this warning? or rewrite your code to have a raise statement and a range check.