Type inference that worked on 0.3.17 fails on versions ≥ 0.3.19

[nmathewson]

Hello!

It looks like the following code, which used to work fine on time 0.3.17, now fails on 0.3.18 and 0.3.19:

use std::time::SystemTime;
use time::ext::NumericalDuration;
use time::OffsetDateTime;

fn main() {
    let now = OffsetDateTime::now_utc();
    let one_hour = 1.hours();

    let _: SystemTime = (now+one_hour*2).into();
}

It gives this error:

   Compiling test-time v0.1.0 (/home/nickm/src/test-time)
error[E0282]: type annotations needed
 --> src/main.rs:9:25
  |
9 |     let _: SystemTime = (now+one_hour*2).into();
  |                         ^^^^^^^^^^^^^^^^ cannot infer type

The last line works fine if you remove the * 2, or if you replace one_hour * 2 with 2.hours(), or if you replace the whole expression with SystemTime::from(now+one_hour*2).

[jhpratt]

I don't immediately recall a reason this would have changed. I'll look into it.

It's worth noting that this is a minor breaking change per RFC 1105.

[jhpratt]

Bisect traces this to ce6c5af, which was not supposed to affect the public API in any way. Time for a more thorough investigation…

[jhpratt]

Oddly, adding a type to the integer being multiplied solves the inference problem.

Slightly modifying the example to better demonstrate the issue expression-by-expression.

use std::time::SystemTime;
use time::ext::NumericalDuration;
use time::OffsetDateTime;

fn main() {
    let one_hour = 1.hours();
    let two_hours = one_hour * 2;
    let added = OffsetDateTime::UNIX_EPOCH + two_hours;
    let _: SystemTime = added.into();
}

error[E0282]: type annotations needed
 --> src/main.rs:8:9
  |
8 |     let added = OffsetDateTime::UNIX_EPOCH + two_hours;
  |         ^^^^^
9 |     let _: SystemTime = added.into();
  |                         ----- type must be known at this point
  |
help: consider giving `added` an explicit type
  |
8 |     let added: /* Type */ = OffsetDateTime::UNIX_EPOCH + two_hours;
  |              ++++++++++++

Either of the following changes makes compilation succeed:

-   let two_hours = one_hour * 2;
+   let two_hours = one_hour * 2i32;

-   let _: SystemTime = added.into();
+   let _ = SystemTime::from(added);

With my understanding of the compiler, neither of these changes should make a difference. The trait implementations are the same before and after ce65af. At this point, this is beyond my level of compiler knowledge. I'm going to create a thread on Zulip to see if anyone with more knowledge can figure this out.

[LegionMammal978]

There is one difference in the trait implementations that looks like it could be relevant here:

-impl<T> Add<T> for OffsetDateTime
-where
-    PrimitiveDateTime: Add<T, Output = PrimitiveDateTime>,
-{
-    type Output = Self;
-
-    fn add(self, rhs: T) -> Self::Output {
-        (self.local_datetime + rhs).assume_offset(self.offset)
-    }
-}
+impl Add<Duration> for OffsetDateTime {
+    type Output = Self;
+
+    fn add(self, rhs: Duration) -> Self::Output {
+        Self(self.0.add(rhs))
+    }
+}
+
+impl Add<StdDuration> for OffsetDateTime {
+    type Output = Self;
+
+    fn add(self, rhs: StdDuration) -> Self::Output {
+        Self(self.0.add(rhs))
+    }
+}

I think this is what is tripping up the type inference. The variables have the types:

one_hour: Duration
two_hours: <Duration as Mul<{integer}>>::Output
added: <OffsetDateTime as Add<<Duration as Mul<{integer}>>::Output>>::Output

In the old version, with the single blanket impl, the compiler apparently feels safe working back and substituting {integer} with an implicit i32, then working forward and figuring out the rest of the types. But in the new version, with the two concrete impls, it apparently doesn't want to resolve the {integer} in the same way. Even though the old version ultimately redirects to two concrete impls, funneling it through one blanket impl makes it work.

For a smaller example:

use std::ops::{Add, Mul};

struct D;
struct D2;
struct ODT;
struct ST;

impl Mul<i32> for D {
    type Output = D;
    fn mul(self, _: i32) -> Self::Output {
        Self
    }
}

impl Mul<i64> for D {
    type Output = D;
    fn mul(self, _: i64) -> Self::Output {
        Self
    }
}

impl Add<D> for ODT {
    type Output = ODT;
    fn add(self, _: D) -> Self::Output {
        Self
    }
}

impl Add<D2> for ODT {
    type Output = ODT;
    fn add(self, _: D) -> Self::Output {
        Self
    }
}

impl From<ODT> for ST {
    fn from(_: ODT) -> Self {
        Self
    }
}

fn main() {
    let one_hour: D = D;
    let two_hours = one_hour * 2;
    let added = ODT + two_hours;
    let _: ST = added.into();
}

Commenting out the impl Add<D2> for ODT block allows the code to compile.

[QuineDot]

(Pretty much the same conclusions as above:)

---

I believe it was the change from

impl<T> Add<T> for OffsetDateTime
where
    PrimitiveDateTime: Add<T, Output = PrimitiveDateTime>,
{ /* ... */ }

to

impl Add<Duration> for OffsetDateTime { /* ...*/ }
impl Add<StdDuration> for OffsetDateTime { /* ...*/ }

Here's a playground reproduction to more easily swap between the two (not necessarily minimal).

If you can restore the indirect implementation, it may solve the regression.

---

I haven't nailed down the why. Spitballing: The compiler defers knowing the type of two_hours (ambiguous integer, doesn't see all Mul implementations have the same Output) or added (ambiguous two_hours) until forced to, and there's something in the trait resolution that triggers a fallback to i32 with the generic implementation that doesn't get triggered with the new implementations. (Once you've made the integer unambiguous, two_hours is unambiguous, so added is unambiguous.)

Here's another way to work around it:

-   let _: SystemTime = added.into();
+   let _: SystemTime = <_>::into(added);
+   // or
+   // let _: SystemTime = Into::into(added);

The latter will only find trait methods.

[QuineDot]

Reduction.

#23545 I suppose. The <_>::-like workarounds work because they avoid method call resolution, based Niko's first comment in that issue.

After thinking about it a little more, I believe the difference is the number of implementations. With the generic approach, there's only one applicable implementation. This could lead the compiler consider added to have an unambiguous type, so the .into() is no longer problematic. (For example this fails too.) Then it can get to "the end of the cycle" and applies integer fallback.

In that case, the compiler noticing multiple implementations have the same resulting type would also fix this.

[jhpratt]

Thank you for the detailed explanations!

I'll look into whether it's reasonable to revert the implementation to a blanket impl. It is a breaking change, although nobody seems to have been relying on it. I'll have to weigh the future API design versus this being a tree falling without making a noise (directly).

This specific inference issue is definitely upstream in the compiler, and the inference failure is a minor breaking change as noted above. So the inference by itself is not sufficient reason to revert the change.

[jhpratt]

While it is theoretically possible to revert the change in question, I have decided not to do so. Doing so would result in worse code overall, both in time and in code generated (the assembly is ~50% larger for the method if I revert it). It would also somewhat restrict API development of time, as it would functionally not allow exposing the current DateTime struct that is presently internal.

As the inference is acceptable breakage per RFC 1105, that's a known issue upstream in the compiler. I'm sure eventually inference will be improved, though I am not aware of any concrete plans to implement the relevant logic.

In any situation, thank you for the report! It's best to be sure when something breaks unexpectedly.