Time::Piece overloading will not accept overloaded objects

[mla]

Discovered that bignum does not play nice with Time::Piece. Getting "Invalid rhs of addition: 46800" errors when the seconds value is a Math::BigInt::Lite instance.

Example:

#!/usr/bin/env perl

  use strict;
  use warnings;
  use Time::Piece qw/ localtime /;
  use Time::Seconds;

  my $d1 = localtime;
  print "\$d1 is a ", ref($d1), "\n";

  my $secs = 46800;
  print "\$secs is a ", (ref($secs) || 'scalar') , "\n";

  my $d2 = $d1 + $secs;
  print "sum is: $d2\n\n";

  use bignum;

  $secs = 46800;
  print "\$secs is a ", (ref($secs) || 'scalar') , "\n";

  $d2 = $d1 + $secs;
  print "sum is: $d2\n";

There's an explicit check for a ref on rhs, in which case the value is rejected. From Time::Piece 1.3204:

  sub add {
      my $time = shift;
      my $rhs = shift;
      if (UNIVERSAL::isa($rhs, 'Time::Seconds')) {
          $rhs = $rhs->seconds;
      }
      croak "Invalid rhs of addition: $rhs" if ref($rhs);

      return $time->_mktime(($time->epoch + $rhs), $time->[c_islocal]);
  }

I can force the BigInt to an int, of course, but would be nice if it was more transparent. I ran into this when a "use bignum" was added to a module and it unexpectedly broke some of the date handling.

I have a proof-of-concept here: https://github.com/Dual-Life/Time-Piece/compare/master...mla:feature/blessed?expand=1

The _mktime logic is a bit tricky, since Time::Piece is itself a blessed array ref. So it has to know when it should or shouldn't try to expand the contents of $time.

My version seems to be passing all tests and satisfies my use-case. I haven't tested it for all overloaded operators or added any tests. It also requires Scalar::Util now; not sure if it's a problem to add that dependency to a core module or not.

[smith153]

I don't think adding Scalar::Util is an issue, looks like it was added to core in 5.8. But that brings the question, perhaps we should be using looks_like_number $rhs instead of looking at the ref as for the most part we either want a Time::Seconds instance or a number?

Looking at _mktime, what you have will probably work. But there are other modules out there under the Time::Piece::* namespace so I should probably test all of those and the reverse dependencies (200+). And if I am going to do that, I might as well refactor _mktime a bit as I've never liked the wantarray call in there since wantarray is never true. At least not while running the test suite anyway.... So I'll add those few things, test all the rev deps and post an update in a couple of days :+1:

[mla]

Great, thanks. And yes, looks_like_number looks like it would work great AFAICT.

#!/usr/bin/env perl

use strict;
use warnings;
use Math::BigInt;
use Math::BigInt::Lite;
use Scalar::Util qw/ reftype looks_like_number /;
use Time::Piece;

my @vars = (
  46800,
  scalar localtime,
  Math::BigInt->new('46800'),
  Math::BigInt::Lite->new('46800'),
);

foreach my $var (@vars) {
  print "\$var = $var\n";
  print "is a ", ref($var), "\n";
  print "looks like a number? ",
        (looks_like_number($var) ? 'Yes' : 'No'),
        "\n\n";
}

$var = 46800
is a 
looks like a number? Yes

$var = Tue Jul 10 21:55:19 2018
is a Time::Piece
looks like a number? No

$var = 46800
is a Math::BigInt
looks like a number? Yes

$var = 46800
is a Math::BigInt::Lite
looks like a number? Yes

[mla]

So we can potentially simplify add() to just:

sub add {
    my $time = shift;
    my $rhs = shift;

    looks_like_number $rhs or croak "Invalid rhs of addition: $rhs";
    return $time->_mktime(($time->epoch + $rhs), $time->[c_islocal]);
}

https://github.com/Dual-Life/Time-Piece/compare/master...mla:feature/blessed?expand=1

[smith153]

yes indeed. We can't forget about subtract() either :)

[mla]

Comment from Dan Book:

I don't think we need looks_like_number or reftype here either. reftype will violate the encapsulation of an object that's passed, you should only check that if it's not blessed (in which case you can just just check if ref eq 'ARRAY'). The looks_like_number check shouldn't be necessary (looks_like_number has some interesting edge cases), you can just use it as a number and depend on appropriate overloading, Perl will emit the appropriate warning or error if it doesn't end up being numeric. Time::Seconds's overload means we don't need to special case that as you correctly fixed.

[mla]

So @Grinnz, you're referring to _mktime here?

Yes, I may have gone overboard with the looks_like_number. I just found it appealing that it seems to do the right thing for overloaded objects.

For _mktime specifically, I think we need to distinguish between a Time::Piece-like instance (which currently is expanded as an arrayref based on being a ref), and other objects (like Math::BigInt) which evaluate to an epoch seconds.

What do you think of this earlier version? https://github.com/mla/Time-Piece/commit/f3d3308405374fb3dd67282e92ca5d10636c8717

What edge cases do we need to worry about with looks_like_number? inf and such?

[Grinnz]

One of the things I'm concerned about with the looks_like_number approach is that it will test the 0+ overload, whereas objects can separately overload + and -, and those are the ones we're interested in here. The edge cases themselves shouldn't actually be a problem here. The main question is just whether it's worth throwing an exception at all when someone tries to add 'foo' to it, instead of just letting it add 0 and throw a warning like Perl usually does.

I believe the Time::Piece special case is still present? It does need to handle that specifically. Any other blessed object can just be treated as a number of seconds, it's up to the user to make sure the object they add evaluates to that.

[Grinnz]

The earlier version looks ok, but UNIVERSAL::isa is discouraged, instead you should check (blessed $time && $time->isa('Time::Piece'))

[smith153]

I didn't really see the point of the ref check in add() either. We only care if its a Time::Seconds object for the most part.

[mla]

I didn't really see the point of the ref check in add() either. We only care if its a Time::Seconds object for the most part.

Yes, and that was, after all, one of the things that prevented the use of overloaded values if the first place.

Ok. Made those changes to my branch. Removed all the looks_like_number usage. Removed the ref checks. And replaced the UNIVERSAL::isa calls.

Does the _mktime class name stuff look needlessly complex?

    $class = eval { (ref $class) && (ref $class)->isa('Time::Piece') }
           ? ref $class
           : $class;

Wouldn't just the old: $class = ref($class) || $class work fine there? Why do we need to worry that it's a Time::Piece?

[Grinnz]

That code is specifically looking for whether $class is a class name or a Time::Piece object. If it's for example an unblessed arrayref, using 'ARRAY' as the class name would not be appropriate (the stringified arrayref wouldn't either but that's the caller's fault).

But the eval isn't necessary nor is the ref call, now that we're importing blessed. It can be simplified to a similar check to the others: $class = (blessed $class && $class->isa('Time::Piece')) ? ref $class : $class;

And you're right there's really no point in checking if it's a Time::Piece object at all; we can assume any blessed object it's invoked on has an appropriate class name to use. That simplifies it to $class = blessed($class) || $class;

[mla]

How would $class ever be an unblessed arrayref though? I understand $time. That's a normal parameter. But how would _mktime even get called with $lcass as an unblessed arrayref? We have callers using it as a function or something?

[Grinnz]

Sorry I didn't mean to imply it would happen in sensible code, it was just an example :)

[mla]

I added some tests and changed the constructor to accommodate overloaded values too (using same approach, which I factored out into a private method).

@smith153 what's involved in testing the 200+ dependent modules? Is that automated? Can I help?

[smith153]

@mla Thanks for the tests, those are always good.

As far as dependent modules, it is not really automated. I have a somewhat complete list of dependent modules in reverse_deps.txt. I should probably prune it a bit as there are many modules in that list that are simply broken and won't install anyway.

But my normal routine is to install the current dev version of Time::Piece, then use a script I have to attempt to install each one of those modules in that list. Once that completes, I revert the Time::Piece version to the current stable and then run the script again. If a module that previously failed to install, suddenly installs, then I'll look at that one more closely. Someone has tried to automate dependency checking: http://blogs.perl.org/users/steve_bertrand/2017/12/verifying-your-distributions-revdeps-still-work-after-a-change-1.html but I don't know if that will make it any easier. For the most part the trouble makers are the Date:: and Time:: modules as many of them inherit or sub class Time::Piece and do no telling what. I'm certainly open to ideas though.

I'll probably get around to it today I think.

[smith153]

I've committed the changes along with some of my own to a local branch, waiting on testing. I spent a few hours last week trying to figure out how to update the reverse_deps.txt file that my scripts use but since the metacpan api changed, none of that was working. https://fastapi.metacpan.org/v1/reverse_dependencies/dist/Time-Piece shows a count of 187 but only lists 49 while https://metacpan.org/requires/module/Time::Piece shows somewhere around 180 while the "cpan river" chart shows only 137 "direct dependents". I haven't figured out yet where to get this info...

[smith153]

I had a few failures in some dependent dists but it was due to my own changes. I've reverted those, and will test again soon.

[smith153]

Pushed version 1.33 to cpan

[mla]

:+1: