Locals stored as private symbols on non-native JS types breaks JS semantics

[steinybot]

Scastie: https://scastie.scala-lang.org/thK7wor1S7OeKO9smIZx9Q
Repo: https://github.com/steinybot/bug-reports/tree/scala-js/combine-objects-private-fields

Attempting to run:

package example

import scala.scalajs.js

object Main extends App {

  trait Config extends js.Object {
    def title(): String
  }

  val config = {
    // Inlining this fixes the problem.
    // Moving it out of the current block scope also fixes the problem.
    // It is getting stored as a private symbol on the config object.
    val routeTitle = "My page"
    val config = new Config {
      override def title(): String = routeTitle
    }
    // This causes the private symbols to be lost.
    // It is a js.Object though so JavaScript semantics should apply.
    js.Object.assign(js.Object(), config).asInstanceOf[Config]
  }

  println(config.title())
}

fails with:

sbt:bug-reports> run
[info] compiling 1 Scala source to /Users/jason/src/bug-reports/target/scala-2.13/classes ...
[info] Fast optimizing /Users/jason/src/bug-reports/target/scala-2.13/bug-reports-fastopt
[info] Running example.Main.
TypeError: Cannot read properties of undefined (reading 'routeTitle$1')
    at Object.title (file:///Users/jason/src/bug-reports/target/scala-2.13/bug-reports-fastopt/example.-Main$.js:33:232)
    at $c_Lexample_Main$.delayedEndpoint$example$Main$1__V (file:///Users/jason/src/bug-reports/target/scala-2.13/bug-reports-fastopt/example.-Main$.js:39:104)
    at $c_Lexample_Main$delayedInit$body.apply__O (file:///Users/jason/src/bug-reports/target/scala-2.13/bug-reports-fastopt/example.-Main$delayed-Init$body.js:18:110)
    at Module.$f_s_App__main__AT__V (file:///Users/jason/src/bug-reports/target/scala-2.13/bug-reports-fastopt/internal-3ebfae0cba70adf981029a0da5b1e4b5ab5d02c6.js:2037:12)
    at Module.$s_Lexample_Main__main__AT__V (file:///Users/jason/src/bug-reports/target/scala-2.13/bug-reports-fastopt/example.-Main.js:6:60)
    at file:///Users/jason/src/bug-reports/target/scala-2.13/bug-reports-fastopt/main.js:4:26
    at ModuleJob.run (node:internal/modules/esm/module_job:222:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:323:24)
[error] org.scalajs.jsenv.ExternalJSRun$NonZeroExitException: exited with code 1
[error] 	at org.scalajs.jsenv.ExternalJSRun$$anon$1.run(ExternalJSRun.scala:200)
[error] stack trace is suppressed; run last Compile / run for the full output
[error] (Compile / run) org.scalajs.jsenv.ExternalJSRun$NonZeroExitException: exited with code 1
[error] Total time: 3 s, completed 21/03/2024, 12:13:20 pm

The issue seems to be the way that routeTitle is being stored on the config object within the PrivateFieldsSymbolHolder property which is non-enumerable. When the values from config are copied over to the new object this non-enumerable property is lost. Then when we invoke the title function the implementation attempts to get the PrivateFieldsSymbolHolder but it is undefined.

$c_Lexample_Main$.prototype.delayedEndpoint$example$Main$1__V = (function() {
  var config = (() => {
    var routeTitle$1 = null;
    routeTitle$1 = "My page";
    var this$1 = ({});
    Object.defineProperty(this$1, $j_internal$002d3ebfae0cba70adf981029a0da5b1e4b5ab5d02c6.$m_sjsr_PrivateFieldsSymbolHolder$().sjsr_PrivateFieldsSymbolHolder$__f_privateFieldsSymbol, ({
      "value": ({
        "example.Main$$anon$1::routeTitle$1": null
      })
    }));
    this$1.title = (function() {
      return $j_internal$002d3ebfae0cba70adf981029a0da5b1e4b5ab5d02c6.$as_T(this[$j_internal$002d3ebfae0cba70adf981029a0da5b1e4b5ab5d02c6.$m_sjsr_PrivateFieldsSymbolHolder$().sjsr_PrivateFieldsSymbolHolder$__f_privateFieldsSymbol].routeTitle$1);
    });
    this$1[$j_internal$002d3ebfae0cba70adf981029a0da5b1e4b5ab5d02c6.$m_sjsr_PrivateFieldsSymbolHolder$().sjsr_PrivateFieldsSymbolHolder$__f_privateFieldsSymbol].routeTitle$1 = routeTitle$1;
    return this$1;
  })();
  this.Lexample_Main$__f_config = Object.assign(Object(), config);
  var x = $j_internal$002d3ebfae0cba70adf981029a0da5b1e4b5ab5d02c6.$as_T(this.Lexample_Main$__f_config.title());
  var this$6 = $j_internal$002d3ebfae0cba70adf981029a0da5b1e4b5ab5d02c6.$m_s_Console$();
  var this$7 = $j_internal$002d3ebfae0cba70adf981029a0da5b1e4b5ab5d02c6.$n(this$6.out__Ljava_io_PrintStream());
  this$7.java$lang$JSConsoleBasedPrintStream$$printString__T__V((x + "\n"));
});

You could argue that this is not breaking JS semantics because non-enumerable properties are never copied. You could run into the same thing with JS:

const config = {
  title: function() {
    return this.routeTitle;
  }
};

Object.defineProperty(config, 'routeTitle', {
  value: "My page",
  writable: false,
});

console.log(config.title())
console.log({...config}.title()) // undefined

The issue is that as a programmer we did not choose to use a non-enumerable property to store the local value, that is implementation specific. It seems like a particularly difficult foot gun to track down (took me a few hours).

In this particular case, using PrivateFieldsSymbolHolder seems to be redundant. A closure would capture the value just fine.

Is there a reliable way to workaround this and prevent it from being stored on the PrivateFieldsSymbolHolder?

[steinybot]

This seems to workaround the problem:

  private val privateFieldsSymbol = {
    // Check that there is only one private fields symbol.
    // This might break if Scala.js changes the way this is implemented and we want to know when that happens.
    val first = scala.scalajs.runtime.privateFieldsSymbol()
    val second = scala.scalajs.runtime.privateFieldsSymbol()
    if (first != second) throw new UnsupportedOperationException("Unable to determine privateFieldSymbol.")
    first.asInstanceOf[String | js.Symbol]
  }

  private def isObject(a: Any): Boolean =
    a != null && js.typeOf(a) == "object"

  private def combinePropertyDescriptor(first: PropertyDescriptor, second: PropertyDescriptor): PropertyDescriptor = {
    // This only updates the value on the descriptor not the property on the object that the descriptor came from.
    val newValue = first.value.fold(second.value) { firstValue =>
      if (isObject(firstValue) && isObject(second.value)) {
        combineOwnProperties[js.Object, js.Object](firstValue.asInstanceOf[js.Object], second.value.asInstanceOf[js.Object])
      } else {
        second.value
      }
    }
    combineOwnProperties(second, new PropertyDescriptor {
      value = newValue
    })
  }

  /**
   * Combines all the own properties – enumerable and not enumerable, strings and symbols — of two objects.
   *
   * Properties from the `second` object will be created after properties from the `first`. When a property is not
   * unique, the one from the `second` object will replace the one from the `first`.
   *
   * @return a new object with the combined properties from both the `first` and `second` objects
   */
  def combineOwnProperties[A <: js.Object, B <: js.Object](first: A, second: B): A with B = {
    val result = js.Object()
    val firstKeys = Reflect.ownKeys(first)
    val secondKeys = Reflect.ownKeys(second)
    val secondKeysSet = secondKeys.toSet
    // We want to be careful with property order.
    // Some things may provide a deterministic order in the order that the properties are created.
    // All unique keys from the first object will retain their relative order and will come before all other keys.
    // Keys from the second object (unique or not) will retain their relative order.
    // This means that the position of non-unique keys will move position (unless they are all at the end of the first
    // and the start of the second).
    firstKeys.foreach { key =>
      if (!secondKeysSet.contains(key)) {
        val descriptor = Reflect.getOwnPropertyDescriptor(first, key).get
        Reflect.defineProperty(result, key, descriptor)
      }
    }
    secondKeys.foreach { key =>
      val maybeFirstDescriptor = Reflect.getOwnPropertyDescriptor(first, key)
      val secondDescriptor = Reflect.getOwnPropertyDescriptor(second, key).get
      val descriptor = maybeFirstDescriptor.fold(secondDescriptor) { firstDescriptor =>
        if (key == privateFieldsSymbol) combinePropertyDescriptor(firstDescriptor, secondDescriptor)
        else secondDescriptor
      }
      Reflect.defineProperty(result, key, descriptor)
    }
    result.asInstanceOf[A with B]
  }

[sjrd]

AFAICT, this is as designed. The new Config { ... } requires some private field to hold routeTitle. We don't promise that only visible fields will be used to implement a JS class. The compiler is free to create additional private fields.

If you want absolute control over the internals of the produced object, you can create as a js.Dynamic.literal(...) and fill in all the members yourself. That would be much more robust than fiddling with the internal structure after the fact. Note that scala.scalajs.runtime is not part of the public API (it's not shown in Scaladoc), just like scala.runtime.

[gzm0]

My 2 cts here: I do agree that the current behavior violates the principle of least surprise.

In fact, I do not understand why we need the private field:

object HelloWorld {
  trait Config extends js.Object {
    def title(): String
  }

  def config() = {
    // Inlining this fixes the problem.
    // Moving it out of the current block scope also fixes the problem.
    // It is getting stored as a private symbol on the config object.
    val routeTitle = "My page"
    val config = new Config {
      override def title(): String = routeTitle
    }

    config
  }
}

Compiles to:

module class helloworld.HelloWorld$ extends java.lang.Object {
  def config;Lhelloworld.HelloWorld$Config(): any = {
    val routeTitle: java.lang.String = "My page";
    val config: any = (arrow-lambda<superClass$: any = constructorOf[scala.scalajs.js.Object], routeTitle$1$2{routeTitle$1}: java.lang.String = routeTitle>(): any = {
      var routeTitle$1: java.lang.String = null;
      val overload: int = {
        routeTitle$1 = routeTitle$1$2;
        0
      };
      val this{this}: any = {};
      global:Object["defineProperty"](this, mod:scala.scalajs.runtime.package$.privateFieldsSymbol;Ljava.lang.Object(), {
        "value": {
          "helloworld.HelloWorld$$anon$1::routeTitle$1": null
        }
      });
      this["title"] = (lambda<>(): any = {
        helloworld.HelloWorld$$anon$1::title;Lhelloworld.HelloWorld$$anon$1;Ljava.lang.String(this)
      });
      this[mod:scala.scalajs.runtime.package$.privateFieldsSymbol;Ljava.lang.Object()]["routeTitle$1"] = routeTitle$1;
      (void 0);
      this
    })();
    config
  }
  constructor def <init>;V() {
    this.java.lang.Object::<init>;V();
    <storeModule>
  }
}

But the following would be clearly better:

module class helloworld.HelloWorld$ extends java.lang.Object {
  def config;Lhelloworld.HelloWorld$Config(): any = {
    val routeTitle: java.lang.String = "My page";
    val config: any = (arrow-lambda<superClass$: any = constructorOf[scala.scalajs.js.Object], routeTitle$1$2{routeTitle$1}: java.lang.String = routeTitle>(): any = {
      var routeTitle$1: java.lang.String = null;
      val overload: int = {
        routeTitle$1 = routeTitle$1$2;
        0
      };
      val this{this}: any = {};
      this["title"] = (lambda<routeTitle$3 = routeTitle$1>(): any = {
        routeTitle$3
      });
      (void 0);
      this
    })();
    config
  }
  constructor def <init>;V() {
    this.java.lang.Object::<init>;V();
    <storeModule>
  }
}

I realize that this will need much more massaging the Scala compiler. But is there ever a case where a capture in Scala cannot be compiled to a closure or class capture in Scala.js IR?