Unsafe conversion of units possible

[bbarker]

IMO, it shouldn't be easily possible to transform a Time value into another Time value that represents a different quantity. I accidentally wrote something analogous to the following in a large code base, which, led me on a bit of a chase:

import squants.time.Time
import squants.time.TimeConversions._

class Main{}
object Main {

  def main(args: Array[String]): Unit = {
    val initTime: Time = 1.0 seconds
    val initTime2: Time = initTime.seconds
    println(initTime)
    println(initTime2)
  }
}

This yields as output:

1.0 s
1000.0 s

I assume this compiles because Time is also a Numeric, and compiling is in principle fine - but not sure why it changed in magnitude. However, it might be even more unsafe if I was using a unit that wasn't default in the conversion, then I would expect it would definitely change, even though it shouldn't.

I'm not sure if it is possible to handle this at the type level off-hand, but throwing a warning or Exception when this occurs might be another possibility.

[garyKeorkunian]

@bbarker You are correct. This issue is the result of the Numeric implementation. The behavior is described in Issue #103.

It is a significant issue.

I think the real solution to this is found #15. The intended solution would introduce a new type of numeric (SquantsNumeric) that would be used initialize quantity values and deprecate using Numeric for that purpose (QuantityNumerics could still be used for their intended purpose). It's something I've been toying with off and on since this project began. It's not an easy implementation - it's like type system whack-a-mole. If anyone is interested they can check out the experimental package in the test code.

In the meantime, perhaps there is something else we can do flag these misuses someway. Let's leave this issue open and volley some ideas.

[bbarker]

I agree that a solution of some sort in the 1.x series would be desirable; both for immediate use, and for cases where one might not have time to upgrade to 2.x immediately (depending on the magnitude of API changes).

I wonder if an "early catch and fail" solution might work at runtime. For instance, in the following snippet, a check could be done to see if num extends AbstractQuantityNumeric - if it does, then a runtime warning or failure could be emitted:

object Mass extends Dimension[Mass] with BaseDimension {
  private[mass] def apply[A](n: A, unit: MassUnit)(implicit num: Numeric[A]) =

Since we don't want such overhead in runtime, maybe this could be added in a "development mode" - not sure the best way to do this in general or for the JVM, but Scala.js is one example where two different optimization levels exist.

It might also be possible to avoid all this runtime ugliness and constrain num to be: num: T such that T <: Numeric[A] and T !<: AbstractQuantityNumeric[A] ... unfortunately, I have no idea if this is possible, and the !<: seems to be fictional - not sure if there is some type magic to make this work. I recently came across a related example of how to create a set-theoretic difference constrain on types (in this example, foo will not accept Unit or Null).

[garyKeorkunian]

We could assert that the seed value is not another Quantity:

  private[mass] def apply[A](n: A, unit: MassUnit)(implicit num: Numeric[A]) = {
    assert(!n.isInstanceOf[Quantity[_]], "A Quantity's seed value may not be another Quantity")
    new Mass(num.toDouble(n), unit)
  }

But, of course, that doesn't type check at compile time, which is most desirable. After all, the goal of the library is dimensional type safety.

I agree that a !<: operator would be nice.

Version 2 will replace the implicit Numeric in the apply method with a new QuantityNumeric. Since these type classes won't be defined for Quantities (only non-dimensional numeric types), attempting to a Quantity as a seed value for another won't compile.

In the meantime, I think the assertion is better than nothing. Developers should not be using Quantities as seed values for other quantities. If they have good test coverage, these errors should be resolved before merged to their code base.

[bbarker]

I think that assertion would be great! I also agree tests are helpful in just the way you describe (incidentally, some of my errors were actually in the tests - it is just a bit too easy to make a typo when you forget if some value is a Double or if it is already a Quantity). Typos happen, sometimes one has to read code ;). But in this case I agree the assert would make for less reading!

[derekmorr]

I really don't like the idea of making the apply functions partial or of using recursion in a critical path.

[garyKeorkunian]

@derekmorr Do you mean placing an assert in the apply method?

[bbarker]

Since we can work with n: A directly, which is slightly simpler than working with num, I worked from the example I linked above to create a !<: trait that seems to have the desired behavior.

This might let us do something like:

  private[mass] def apply[A](n: A, unit: MassUnit)
  (implicit num: Numeric[A], implicit not: A !<: Quanity[_]) = 
    new Mass(num.toDouble(n), unit)

[bbarker]

Any other thoughts on the last proposal I mentioned using the custom !<: trait? I can give it a try if it looks acceptable.

[derekmorr]

There's a similar but different approach proposed by Miles Sabin - https://gist.github.com/milessabin/c9f8befa932d98dcc7a4 it doesn't rely on casting.

[bbarker]

That looks good to me ... Hadn't thought of using null. Don't think we need to bother with the type alias though, since we're not going to be using this much.

On Jul 11, 2017 9:53 PM, "Derek Morr" notifications@github.com wrote:

There's a similar but different approach proposed by Miles Sabin - https://gist.github.com/milessabin/c9f8befa932d98dcc7a4 it doesn't rely on casting.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/typelevel/squants/issues/230#issuecomment-314621749, or mute the thread https://github.com/notifications/unsubscribe-auth/AA37jgNJ6KeXmOM8Arx1AF33r_oOidv6ks5sNCcbgaJpZM4NPoi6 .