Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make native function signatures type-safe, fix broken SDK.Basics ops #363

Merged
merged 3 commits into from
Aug 30, 2023

Conversation

deusaquilus
Copy link
Contributor

@deusaquilus deusaquilus commented Aug 30, 2023

Two major things have been done here, evaluator fixes and the basic implementation of higher-order native functions.

Evaluator Fixes and Improvements

  • Extremely problematic constructs were being used inside of evaluator e.g:
    val plus: SDKValue[Unit, Type.UType] = SDKValue.SDKNativeFunction(
      2,
      (a: Result[Unit, Type.UType], b: Result[Unit, Type.UType]) =>
        Result.Primitive(Result.unwrap(a).asInstanceOf[Long] + Result.unwrap(b).asInstanceOf[Long])
    )
  • These mistakes were based on a bad implementation of Result.Primitive which allowed Any as a value.
    case class Primitive[TA, VA](value: Any) extends Result[TA, VA] 
    This has been fixed and now supports an enumeration of values:
    Int[TA, VA](value: scala.Int) extends Numeric[TA, VA, scala.Int]
    Long[TA, VA](value: scala.Long) extends Numeric[TA, VA, scala.Long]
    Double[TA, VA](value: scala.Double) extends Numeric[TA, VA, scala.Double]
    BigDecimal[TA, VA](value: scala.BigDecimal) extends Numeric[TA, VA, scala.BigDecimal]
    Float[TA, VA](value: scala.Float) extends Numeric[TA, VA, scala.Float]
  • This has resulted in various refactorings that significantly improve safety of the evalautor.
  • Additionally, the format of SDKNativeFunction was extremely unsafe:
    case class SDKNativeFunction[TA, VA](arguments: Int, function: Any)   extends SDKValue[TA, VA]
    This essentially means that a user can accidentally put anything inside of the evaluator loop inside of Native.scala.
    This problem was fixed by adding an enumeration of the only functions that are actually possible:
    case class Fun1[TA, VA](f: Result[TA, VA] => Result[TA, VA]) extends NativeFunctionSignature[TA, VA] { val numArgs = 1 }
    case class Fun2[TA, VA](f: (Result[TA, VA], Result[TA, VA]) => Result[TA, VA]) extends NativeFunctionSignature[TA, VA] { val numArgs = 2 }
    case class Fun3[TA, VA](f: (Result[TA, VA], Result[TA, VA], Result[TA, VA]) => Result[TA, VA]) extends NativeFunctionSignature[TA, VA] { val numArgs = 3 }
    case class Fun4[TA, VA](f: (Result[TA, VA], Result[TA, VA], Result[TA, VA], Result[TA, VA]) => Result[TA, VA]) extends NativeFunctionSignature[TA, VA] { val numArgs = 4 }
    case class Fun5[TA, VA](f: (Result[TA, VA], Result[TA, VA], Result[TA, VA], Result[TA, VA]) => Result[TA, VA]) extends NativeFunctionSignature[TA, VA] { val numArgs = 5 }
    SDKNative function now needs to be one of these:
    case class SDKNativeFunction[TA, VA](function: NativeFunctionSignature[TA, VA])
    Also note that the arity function is now gone. We can infer it automatically.
  • Many of these tests required corresponding changes in the EvaluatorMDMTests file.
  • In order to help Native.scala handle operations for different kinds of primitives (e.g. Morphir.SDK.basics.plus), helper functions were introduced into Result.Primitive to retrieve scala.Numeric, and scala.Integral (or scala.Fractional) based on the type. The former can be used to do +/-/* on all our needed types T, the latter is needed to do division (believe it or not scala.Numeric doesn't do that!).
  • In addition, in order to avoid code duplication where two different numerics would need to be checked (to see if they are the same thing), a function unwrapNumericsWithHelper was added together that summons both types T as well as the scala.Numeric[T] instance for them (and Fractional[T] or Integral[T] as well).

Higher Order Native Functions

  • Based on an analysis of what is going on inside of MapImpl.scala it appears to be extremely difficult to use the same techniques to implement more complex functionality such as Dict.filter. It would literally be the Scala-encoded IR for something like:
    filter : (k -> v -> Bool) -> (Dict k v) -> (Dict k v)
    filter isGood dict =
      Dict.fromList (List.filter (\k1 v1 -> isGood k1 v1) (Dict.toList dict))
    Maintaining this in Scala IR-tree form would be extremely difficult to do.
  • Therefore I have decided to implement higher-order functions in Scala directly. They are not stack-safe yet but neither are List.map and Map.map etc... in Scala/Java so I believe we can do this later.
  • In order to allow this, the evaluation must be called inside of the List/Map.map/fliter etc... functions. The only thing that Native.scala functions are missing in this regard are the Store variable.
  • Therefore I have created a NativeFunctionSignatureAdv class that also contains the Store. Any users of it in Native.scala can access the the store in order to evaluate Result values.

@netlify
Copy link

netlify bot commented Aug 30, 2023

Deploy Preview for magical-donut-e8221d ready!

Name Link
🔨 Latest commit 08d83a1
🔍 Latest deploy log https://app.netlify.com/sites/magical-donut-e8221d/deploys/64ef427e9a6e2e0008f1ec4d
😎 Deploy Preview https://deploy-preview-363--magical-donut-e8221d.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@deusaquilus deusaquilus force-pushed the fix-evaluator-major branch 2 times, most recently from 648372f to fee402a Compare August 30, 2023 05:22
else
v.toInt
case _: Primitive[_, _, _] =>
throw new Exception(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be a sublass of MorphirRuntimeError

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the errors thrown in this file should be a sublass of MorphirRuntimeError or EvaluationError (which is also a MorphirRuntime error)

Copy link
Member

@DamianReeves DamianReeves left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@DamianReeves DamianReeves merged commit 0f16202 into finos:main Aug 30, 2023
@@ -132,7 +142,7 @@ object EvaluatorQuick {
case TT.Reference(_, typeName, typeArgs) =>
val lookedUp = dists.lookupTypeSpecification(typeName.packagePath, typeName.modulePath, typeName.localName)
val conceptArgs = typeArgs.map(typeToConcept(_, dists, boundTypes))
lookedUp.getOrElse(throw new Exception(s"Could not find spec for $typeName")) match {
lookedUp.getOrElse(throw new UnexpectedType(s"Could not find spec for $typeName")) match {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a particular error for something being missing, and the return from Dists should be an either with a helpful error type for specifying what's missing

val divide: SDKValue[Unit, Type.UType] =
SDKValue.SDKNativeFunction.fun2 { (a: Result[Unit, Type.UType], b: Result[Unit, Type.UType]) =>
handleSameNumerics(a, b) { (aNum, bNum, helper) =>
// scala.Numeric does not handle division, it's part of scala.Fractional or scala.Integral, need to get the right one
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

divide as specified in morphir.SDK is only for floats - integerDivide is a completely different function

@@ -324,7 +325,7 @@ object EvaluatorQuick {
curry(constructor, mappedArgs)
case (a, b) => V.tuple(Chunk(scalaToIR(a), scalaToIR(b)))
case (a, b, c) => V.tuple(Chunk(scalaToIR(a), scalaToIR(b), scalaToIR(c)))
case other => throw new Exception(s"I don't know how to decompose $other")
case other => throw new UnmatchedPattern(s"I don't know how to decompose $other")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UnmatchedPattern is specifically for pattern matching in terms of morphir - this should be UnsupportedType

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants