fix object assertions for multiply-inherited objects and put it under a flag

add comment

.

Author: lihaoyi-databricks

sjsonnet/src-jvm-native/sjsonnet/Config.scala

@@ -133,4 +133,9 @@ case class Config(
     doc = """Raise an error if import expressions are used without proper parentheses, e.g. import "foo".bar rather than (import "foo").bar"""
   )
   strictImportSyntax: Flag = Flag(),
+  @arg(
+    name = "strict-inherited-assertions",
+    doc = """Properly handle assertions defined in a Jsonnet dictionary that is extended more than once"""
+  )
+  strictInheritedAssertions: Flag = Flag(),
 )

sjsonnet/src-jvm-native/sjsonnet/SjsonnetMain.scala

@@ -216,7 +216,8 @@ object SjsonnetMain {
         strict = config.strict.value,
         noStaticErrors = config.noStaticErrors.value,
         noDuplicateKeysInComprehension = config.noDuplicateKeysInComprehension.value,
-        strictImportSyntax = config.strictImportSyntax.value
+        strictImportSyntax = config.strictImportSyntax.value,
+        strictInheritedAssertions = config.strictInheritedAssertions.value
       ),
       storePos = if (config.yamlDebug.value) currentPos = _ else null,
       warnLogger = warnLogger,

sjsonnet/src/sjsonnet/Evaluator.scala

@@ -513,8 +513,22 @@ class Evaluator(resolver: CachedResolver,
       } else createNewScope(self, sup)
     }
 
-    def assertions(self: Val.Obj): Unit = if (!asserting) {
-      asserting = true
+    def assertions(self: Val.Obj): Unit = if (
+      // We need to avoid asserting the same object more than once to prevent
+      // infinite recursion, but the previous implementation had the `asserting`
+      // flag kept under the object's *instantiation* rather than under the
+      // object itself. That means that objects that are instantiated once then
+      // extended multiple times would only trigger the assertions once, rather
+      // than once per extension.
+      //
+      // Due to backwards compatibility concerns, this fix has to go in under
+      // the flag `strictInheritedAssertions`, to be removed after an appropriate
+      // time period has passed.
+      (settings.strictInheritedAssertions && !self.asserting) ||
+      (!settings.strictInheritedAssertions && !asserting)
+    ) {
+      if (settings.strictInheritedAssertions) self.asserting = true
+      else asserting = true
       val newScope: ValScope = makeNewScope(self, self.getSuper)
       var i = 0
       while(i < asserts.length) {

sjsonnet/src/sjsonnet/Settings.scala

@@ -8,6 +8,7 @@ class Settings(
   val noStaticErrors: Boolean = false,
   val noDuplicateKeysInComprehension: Boolean = false,
   val strictImportSyntax: Boolean = false,
+  val strictInheritedAssertions: Boolean = false,
 )
 
 object Settings {

sjsonnet/src/sjsonnet/Val.scala

@@ -145,6 +145,7 @@ object Val{
                   `super`: Obj,
                   valueCache: mutable.HashMap[Any, Val] = mutable.HashMap.empty[Any, Val],
                   private[this] var allKeys: util.LinkedHashMap[String, java.lang.Boolean] = null) extends Literal with Expr.ObjBody {
+    var asserting: Boolean = false
 
     def getSuper = `super`
 

sjsonnet/test/src/sjsonnet/EvaluatorTests.scala

@@ -383,6 +383,23 @@ object EvaluatorTests extends TestSuite{
       test - assert(evalErr("""{assert false} + {} + {}""").contains("sjsonnet.Error: Assertion failed"))
       test - assert(evalErr("""{} + {assert false} + {}""").contains("sjsonnet.Error: Assertion failed"))
       test - assert(evalErr("""{} + {} + {assert false}""").contains("sjsonnet.Error: Assertion failed"))
+      test - {
+        val problematicStrictInheritedAssertionsSnippet =
+          """local template = { assert self.flag };
+            |{ a: template { flag: true, }, b: template { flag: false, } }
+            |""".stripMargin
+        assert(
+          evalErr(
+            problematicStrictInheritedAssertionsSnippet,
+            strictInheritedAssertions = true
+          ).contains("sjsonnet.Error: Assertion failed")
+        )
+
+        assert(
+          eval(problematicStrictInheritedAssertionsSnippet, strictInheritedAssertions = false) ==
+          ujson.Obj("a" -> ujson.Obj("flag" -> true), "b" -> ujson.Obj("flag" -> false))
+        )
+      }
     }
   }
 }

sjsonnet/test/src/sjsonnet/TestUtils.scala

@@ -4,7 +4,8 @@ object TestUtils {
   def eval0(s: String,
             preserveOrder: Boolean = false,
             strict: Boolean = false,
-            noDuplicateKeysInComprehension: Boolean = false) = {
+            noDuplicateKeysInComprehension: Boolean = false,
+            strictInheritedAssertions: Boolean = false) = {
     new Interpreter(
       Map(),
       Map(),
@@ -14,20 +15,29 @@ object TestUtils {
       new Settings(
         preserveOrder = preserveOrder,
         strict = strict,
-        noDuplicateKeysInComprehension = noDuplicateKeysInComprehension
+        noDuplicateKeysInComprehension = noDuplicateKeysInComprehension,
+        strictInheritedAssertions = strictInheritedAssertions
       )
     ).interpret(s, DummyPath("(memory)"))
   }
 
-  def eval(s: String, preserveOrder: Boolean = false, strict: Boolean = false, noDuplicateKeysInComprehension: Boolean = false) = {
-    eval0(s, preserveOrder, strict, noDuplicateKeysInComprehension) match {
+  def eval(s: String,
+           preserveOrder: Boolean = false,
+           strict: Boolean = false,
+           noDuplicateKeysInComprehension: Boolean = false,
+           strictInheritedAssertions: Boolean = false) = {
+    eval0(s, preserveOrder, strict, noDuplicateKeysInComprehension, strictInheritedAssertions) match {
       case Right(x) => x
       case Left(e) => throw new Exception(e)
     }
   }
 
-  def evalErr(s: String, preserveOrder: Boolean = false, strict: Boolean = false, noDuplicateKeysInComprehension: Boolean = false) = {
-    eval0(s, preserveOrder, strict, noDuplicateKeysInComprehension) match{
+  def evalErr(s: String,
+              preserveOrder: Boolean = false,
+              strict: Boolean = false,
+              noDuplicateKeysInComprehension: Boolean = false,
+              strictInheritedAssertions: Boolean = false) = {
+    eval0(s, preserveOrder, strict, noDuplicateKeysInComprehension, strictInheritedAssertions) match{
       case Left(err) => err.split('\n').map(_.trim).mkString("\n")  // normalize inconsistent indenation on JVM vs JS
       case Right(r) => throw new Exception(s"Expected exception, got result: $r")
     }