Passing compiler Functions/Mixins across different compilations should not be allowed

[ntkme]

See the following code as an illustration of the issue. On sass this "worked correctly", because callables from other function registries with in the same dart/js VM memory space can be directly accessed and called as functions/mixins are never serialized. However, on sass-embedded this is broken, as only the function id is being passed, which is not a real serialization of the function/mixin content, thus it will refer to a function in the current register even if we passed the function from a different registry.

const sass = require('sass-embedded');
// or
// const sass = require('sass');

const outer = `
  @use 'sass:meta';

  @function foo($n) {@return $n + 1}
  a {b: meta.call(bar(meta.get-function('foo')), 0); }
`

const inner = `
  @use 'sass:meta';

  @function foo($n) {@return $n + 2}
  a {b: meta.call(bar(meta.get-function('foo')), 0); }
`

sass.compileString(outer, {
  functions: {
    'bar($arg)': (args) => {
      const outerFn = args[0]
      console.log(sass.compileString(inner, {
        functions: {
          'bar($arg)': (args) => {
            const innerFn = args[0]
            console.log(outerFn.equals(innerFn))
            return outerFn
          }
        },
      }).css)
      return outerFn
    }
  }
})

[ntkme]

This is currently "undefined behavior" that works in dart-sass, but probably it shouldn't even be allowed. There are a few things need to be done to address this:

1. In dart-sass, if user defined callable returns a callable/mixins that has an environment that is not the current environment attached, throw an error.
2. In embedded host protofier, attach the "function registry" (use it as "environment" tracker) as an attribute, and throw an error if user attempts to passing callables across compilation.
3. In embedded host's compiler-function/mixin, the equals function need to compare the attached environment in additional to just id.

[nex3]

The title refers to host functions, but outerFn in your example is a compiler function. This isn't an issue for host functions because they're protofied separately each time they're passed to the compiler.

I don't think we necessarily need to make this as complicated as attaching registries to function objects. All we really need is some tag (that can be completely opaque to the host) indicating to the compiler which compilation a given function belongs to. We could even reserve a few bits in the existing ID to use for this.

Either way we should definitely make it explicit in the JS API specification that passing a function object between compilations is an error.

[ntkme]

The title refers to host functions, but outerFn in your example is a compiler function.

Sorry, that was a typo.

All we really need is some tag (that can be completely opaque to the host)

Exactly. Anything that has 1-1 mapping with each compilation would work. In ruby implementation I just create a blank BasicObject for this purpose.

[nex3]

What I mean is, it doesn't necessarily need to be the host's concern at all. The compiler itself should provide this guarantee.

[ntkme]

I'm not sure if that can be done on compiler side reliably. In theory, the host side can retain a compiler function object in memory indefinitely, so that no matter what kind of bits we add, there is always a risk of collision at some point.

[ntkme]

In additional, I found the compiler has special logic for argument list that may cause problem as well:

dart-sass/lib/src/embedded/protofier.dart

Lines 235 to 238 in e6589fe

	case Value_Value.argumentList:
	if (value.argumentList.id != 0) {
	return _argumentListForId(value.argumentList.id);
	}

In theory, I should be able to save an ArgumentList from compilation A, and then pass it to compilation B. However, currently this will break. - Probably the host should check set the id to 0 before passing argument list to a different compilation.

[nex3]

You're right that if we reserve (for example) eight bytes of the ID for a compilation ID, there will be a chance that you can get overlap once you have 256+ compilations running concurrently. But even then passing functions across compilation contexts will only work randomly and only very infrequently, so there's no real risk that it would hide errors from the user. Even then, we could still handle it on the compiler side by adding a separate CompilerFunction.compilation_id field—we're already effectively assuming that there won't be 4 billion concurrent compilations.

For ArgumentLists, I think the embedded protocol is fine as-is, although we could add a bit more explicit wording around how non-0 IDs should only be allocated by the compiler. The JS API should probably what happens to argument lists passed between compilations, though (the best option there is probably to just mark it as accessed).

[ntkme]

It’s not just concurrent compilations. Even if I do them sequentially, I can save a “compiler function” to a local variable, and then return it in a different compilation much later. Also, in implementation like the ruby host, the next compilation id is reset to 1 every time the compiler becomes idle, it’s not safe even if it’s a separate field.