Fix Time.new and Time.utc

[wooster0]

@Grabli66 discovered a bug in Time.new.

When you do for example this:

puts Time.new(1977, 8.to_i16, 9.to_i16)

then you get this:

0003-11-10 00:00:00

instead of:

1977-08-09 00:00:00

It's probably because of an overflow internally.
I fixed this by restricting year, month and day of Time.new and Time.utc to Int32.

[straight-shoota]

This is not a real fix, but rather a naive workaround. The real issue is that the return value of Time.absolute_days has the integer type of day and thus is subject to overflow. That needs to be fixed. It should always return and calculate with Int32.

[asterite]

I think all of the API methods that accept Int, or don't specify a type, should be changed to Int32. Now that we have implicit conversion of literals it's not a problem anymore. And if you really want to pass an Int16, cast it in the call site. It's safe, it generates less overloads and code will compile faster.

So 👍 to this PR. @r00ster91 Thank you!

[Sija]

I think all of the API methods that accept Int, or don't specify a type, should be changed to Int32. Now that we have implicit conversion of literals it's not a problem anymore.

@asterite Well, in this case shouldn't it rather be UInt8 for day/month and UInt16 for year? Similarly other arguments doesn't need Int32.

[asterite]

@Sija No, Int32 is the default type that should be used everywhere across APIs. All other integer types have very specific use cases (usually related to low-level programming).

[straight-shoota]

Changing unspecific Int types to Int32 because of #6074 is an independent issue which should be addressed with the entire stdlib in mind. Besides that, it won't really work until the next compiler version is released.

And after all, this PR won't fix the core issue that Time.absolute_days doesn't ensure an appropriate return type.

[asterite]

Yeah, it could probably be done in a separate PR, thought it can be done before 0.25.0 because it will be shipped along the new feature.

But even if the automatic cast feature is not shipped (which won't happen), using Int32 by default should be good because integer literals have that type by default.

[bew]

I don't understand the idea of using signed number instead of unsigned where is doesn't make sense to have negative numbers.
I agree about the default number type, but I think it'd be better to have Int32 and UInt32 as basic defaults when we need a number, with strong logic on the sign-ness.
(and maybe introduce an unsigned literal, like 1_u)

[asterite]

@bew I don't think unsigned numbers exist to represent non-negative integer numbers, but just for their math properties (that they wrap, etc.).

Here's one explanation of why it's a bad idea to use unsigned integers: http://www.aristeia.com/Papers/C++ReportColumns/sep95.pdf

I also remember someone "important" said that confusing unsigned numbers with non-negative numbers is bad, but I can't find it...

[bew]

Very interesting, thank you.
But this is C++, a language with automatic conversion from signed to unsigned to signed. (iirc Crystal has that too :/)
I didn't tried anything (yet), but I think that with no signed/unsigned auto conversion, and the type system, we could prevent mistakes like those explained, at compile time. Wouldn't this be great?

[asterite]

The main point is that subtracting two unsigned values a and b, where a < b, results in a positive integer, not a negative one like would happen with signed values, and that and similar mistakes are a disaster and are very unintuive.

It has nothing to do with implicit conversions.

[asterite]

Plus, there isn't really a benefit of using unsigned integers. They are not faster (in LLVM and in the computer they are represented with the same datatype). So "because it can never be negative" is not a good reason to use unsigned integers. In fact, Java doesn't have unsigned integers and 3 billion devices run Java.

[straight-shoota]

This is unsatisfying. I'm not against restricting the arguments to Int32, even without #6074 or this bug. But this is only half-baked and doesn't even fix the real bug in Time.absolute_days.

Now the two constructors restrict the date fields to Int32 but not the time fields. That seems arbitrarily inconsistent. Other methods that receive date fields, such as Time.days_in_month?, should probably use the same argument types for consistency - even if not affected by arithmetic overflow.

[asterite]

I can send a PR to change Int to Int32 everywhere in type restrictions in the std.

But I don't understand. This is an improvement over the old code. It might not fix all problems, but that's how we move in Crystal, fixing things slowly.

[sdogruyol]

I agree with @asterite here. Let's make things work first 👍

[straight-shoota]

@asterite Sure, it's an improvement. But it also creates new needs for improvement that could have been avoided right away with almost no extra effort.

[oprypin]

I think that the direction we want to move in is having more type annotations. The way I view it is that this change would have been incorporated in a later change sooner or later anyway.

[asterite]

In fact, I wanted to open an issue or a discussion about you (community) showing examples of methods where you wouldn't know what type annotation to use. Everyone say "but then you won't be able to be as expressive", but I really want to see those examples. Because otherwise, let's type everything and make the compiler incremental and fast.

[RX14]

@asterite it's not about not knowing the type, it's about not having to write down the type that we know in our head. And it's about not having to rename a bajillion things when we rename a type and such.

I don't want Go with Ruby syntax I want Crystal, and Crystal is beautiful to me.

[asterite]

it's about not having to write down the type that we know in our head

It's of no use if it's just in your head. Programming is not a solo thing.

I want Go with Ruby syntax, blocks and macros 😢

[RX14]

@asterite then go write it. It'll be a lot easier to write something like that from scratch and not call it crystal than to start from crystal.

[faustinoaq]

Because otherwise, let's type everything and make the compiler incremental and fast.

@asterite Interesting, Maybe we can do some kind of pre-compilation from an untyped (inferred) codebase to generate all typed code, just like crystal tool format unformatted code -> formatted code

This way we can deliver shards with faster compile times, keeping the main codebase clean (without too much types), just like @types/mypackage packages in TypeScript, but instead of using a new shard we can create a folder called types/ with all typed code.

WDYT? 😅

[faustinoaq]

So, normal untyped (inferred) code would still compile slow without incremental compilation

And, new code (generated/all typed) code would have fast compilation with incremental builds

WDYT?

[faustinoaq]

The main use-case would be fast compile times on big projects with many shards ✨

[RX14]

@faustinoaq we'd still have to modify the compiler to take advantage of type restrictions to reduce compilation time in the first case.

And if we could do that, we've solved the whole problem already. You've essentially just reformulated and restarted the existing problem.

[wooster0]

@asterite
I'm currently trying to change Int arg type restrictions to Int32 to reduce the amount of generated overloads.
Here is it: wooster0@15712c1
So far I just replaced every single Int argument type restriction by Int32.
Now I'm of course getting loads of no-overload-match-error-messages and I'm not sure for example here:

in src/string/builder.cr:19: no overload matches 'GC.malloc_atomic' with type UInt32
Overloads are:
 - GC.malloc_atomic(size : Int32)
 - GC.malloc_atomic(size : LibC::SizeT)

    @buffer = GC.malloc_atomic(capacity.to_u32).as(UInt8*)
                 ^~~~~~~~~~~~~

Makefile:121: recipe for target '.build/crystal' failed
make: *** [.build/crystal] Error 1

if I should make it an Union: Int32 | UInt32, change it to UInt32, leave it as Int or cast it: .to_i32.
Can you or someone maybe give me some hint?

[asterite]

GC.malloc_atomic should be LibC::SizeT