Caching and non-frozen arrays

[jridgewell]

    > This was a request from @ljharb. I can update the algorithm to iterate code points?

I suppose that works too, though I'd regard it as overkill vs my suggestion.

I decided to just key using the raw property.

In #73 you're keying by the result of DedentStringsArray, which doesn't work because that's a spec-internal List, not an object:

1. Let _raw_ be ? DedentStringsArray(? Get(_t_, *"raw"*)).
1. For each element _e_ of the _dedentMap_, do
  1. If _e_.[[Raw]] is not ~empty~ and SameValue(_e_.[[Raw]], _raw_) is *true*, return _e_.[[Dedented]].

Presumably you meant something like

1. Let _rawProp_ be ? Get(_t_, *"raw"*).
1. For each element _e_ of the _dedentMap_, do
  1. If _e_.[[Raw]] is not ~empty~ and SameValue(_e_.[[Raw]], _rawProp_) is *true*, return _e_.[[Dedented]].
1. Let _raw_ be ? DedentStringsArray(_rawProp_).

That would be an improvement, but still runs into weirdness like:

let x = { raw: ['a'] };
console.log(String.dedent(x)); // 'a', as expected
x.raw[0] = 'b';
console.log(String.dedent(x)); // 'a' - ???

I think you also want to include an IsFrozen check on arg.raw before using the registry at all. That is

1. Let _rawProp_ be ? DedentStringsArray(? Get(_t_, *"raw"*)).
1. If _rawProp_ is not an Object, throw a *TypeError* exception.
1. Let _isFrozen_ be ? TestIntegrityLevel(_rawProp_, ~frozen~).
1. If _isFrozen_ is *true*, then
  1. For each element _e_ of the _dedentMap_, do
    1. If _e_.[[Raw]] is not ~empty~ and SameValue(_e_.[[Raw]], _rawProp_) is *true*, return _e_.[[Dedented]].
1. Let _raw_ be ? DedentStringsArray(_rawProp_).

(You can additionally add a check before doing the lookup in the registry, but it's technically redundant - things can't go from frozen to un-frozen, so if an object isn't frozen it can't be in the registry and the lookup will not find anything.)

I've written this to throw if .raw is not an Object, though if you really want to support primitives you could do

1. If _rawProp_ is not an object, let _isFrozen_ be *true*.
1. Else, let _isFrozen_ be ? TestIntegrityLevel(_rawProp_, ~frozen~).

instead. Though I note that the definition of [[DedentMap]] says it is keyed by objects, so you probably do want the throw.

Originally posted by @bakkot in #72 (comment)

[Josh-Cena]

Could this get some help from array-is-template-object? (Although that proposal seems to not have progressed much...) TestIntegrityLevel invokes too many observable behaviors ([[IsExtensible]] + [[OwnPropertyKeys]] + [[GetOwnProperty]]), while I don't feel like const obj = { raw: Object.freeze(["foo"]) }; String.dedent(tag)(obj) just for the sake of caching is hugely useful. (I don't think people anticipate identity if the tag call is not via a template literal anyway.)

[bakkot]

Could this get some help from array-is-template-object?

Technically yes, but the objects it produces would not count as template objects, which means they wouldn't be cached by other mechanisms similar to this one. Which seems bad - you want stuff to compose.

TestIntegrityLevel invokes too many observable behaviors

Eh, 99%+ of the time it's going to be either "it's not extensible because it's an ordinary object" or "it's frozen because it's a template object", both of which are very cheap to check, so it's not like that's actually going to slow anything down in engines except for the Proxy case, which is slow anyway.

[Josh-Cena]

the objects it produces would not count as template objects

I'm surprised—you mean if you do:

String.dedent(Array.isTemplateObject)`
  foo
`;

The result is expected to be false, even when Array.isTemplateObject ships?

so it's not like that's actually going to slow anything down

That's fair, but the point is that it could be always one internal slot lookup, unless people actually expect the semantic that a frozen raw property behaves exactly like a template array. When I said that, I was actually not thinking about performance, only about what people can observe if they do pass a proxy.

Another point is documentation. Speaking as an MDN writer, I feel more comfortable explaining "String.dedent caches only if you use it as a template tag; calling it as a function doesn't cache" than "String.dedent caches only if it has a frozen raw property", because that sounds like a leak of implementation (i.e. that String.dedent dedents the raw array and cooks it instead of dedenting the cooked array).

I do see that this proposal is seeking stage 3 soon, in which case testing frozeness is still a reasonable solution so as not to be blocked by the other proposal.

[bakkot]

The result is expected to be false, even when Array.isTemplateObject ships?

Yes. If Array.isTemplateObject ships, it will only return true for objects which correspond to literal source text template objects, not anything derived from them, even things derived via built-ins.

When I said that, I was actually not thinking about performance, only about what people can observe if they do pass a proxy.

Eh, I think we shouldn't be worrying too much about what traps people will observe if they pass a Proxy. That's an extremely unusual case, and not worth spending a lot of effort designing around. Generally speaking I think we should pick a design which makes sense for the common case, and only modify it when considering Proxies if their existence is going to make it impossible to do efficiently at baseline tier in the common case - which is often the case, but isn't the case here.

Another point is documentation. Speaking as an MDN writer, I feel more comfortable explaining "String.dedent caches only if you use it as a template tag; calling it as a function doesn't cache" than "String.dedent caches only if it has a frozen raw property", because that sounds like a leak of implementation (i.e. that String.dedent dedents the raw array and cooks it instead of dedenting the cooked array).

Well, the fact that it dedents based on the raw array is relevant anyway if you're talking about invoking it as a function directly (and not as a wrapper on another tag), because in that case you have to pass { raw: stuff }, not stuff. And if you're talking about invoking it as a function directly, I don't think there's anything wrong with saying "it caches if the raw array is frozen, like it is for template literals".

[Josh-Cena]

Oh well, I get the reasoning. In this case testing for frozeness sounds like the best behavior.

About Array.isTemplateObject though, this makes me wonder if a syntax is the best solution for dedent. (I understand TC39 doesn't like that.) Otherwise it's really surprising if a "secure" tag can only work without dedenting.

[bakkot]

Otherwise it's really surprising if a "secure" tag can only work without dedenting.

I have the opposite opinion - it would be surprising if a security feature passed for anything other than the literal text in your program. Even if we did have syntax for String.dedent I don't think we'd want Array.isTemplateObject to pass for things derived from that syntax. You really, really want a feature like isTemplateObject to be as straightforward as possible, not for it to try to get clever and accept things other than the maximally raw input. Interaction of features like that is one of the more common ways to end up with vulnerabilities.

[Josh-Cena]

This is digressing a bit, but in a hypothetical world where dedent uses a sigil, you would probably expect the tag it encloses to receive an actual template array. It's only because String.dedent is a function that we have this function vs. template tag dichotomy.

[bakkot]

This is digressing a bit, but in a hypothetical world where dedent uses a sigil, you would probably expect the tag it encloses to receive an actual template array.

Not in terms of Array.isTemplateObject, no. That's what I'm saying. Array.isTemplateObject should only ever return true for things which are literal source text in your program, not for things derived from that, even things derived via built-in syntax or APIs.

[Josh-Cena]

Hmmm, I see. I need to reread the motivation of that proposal.

[hax]

@Josh-Cena As I understand array-is-template-object is for security check, so not fit for this case.

[rbuckton]

During the plenary today there was a discussion about this issue and whether String.dedent should throw when passed a non-template object.

I have concerns about throwing due to the potential for mixed-mode environments where you might be downleveling tagged templates but could easily still access a String.dedent that may or may not be polyfilled in that environment. If the constraint for cacheability is that "the object and the arrays must be frozen", that is something a transpiler can accomplish (at least, in ES5+). If the constraint is "the object and arrays were produced by the runtime and checked via an internal slot", then there is no way for a transpiler to meet that constraint.

[hax]

@jridgewell Could u add comment here to explain the worst case if we use "only cache frozen" so everyone could understand the situation? Thank you.

[hax]

Note, as the plenary discussion, we have several options:

1. always cache (current proposal)
2. only cache frozen
3. cache frozen, throw if not frozen
4. no cache

My preference is 2>1=3>>4

[jridgewell]

For what it's worth, I consider this to be an edge case. The expectation is that this will be invoked as a real tagged template literal, and even if you have a user provided strings array, that it will not be modified (because that would break every tag function that expects immutability).

If the constraint for cacheability is that "the object and the arrays must be frozen", that is something a transpiler can accomplish (at least, in ES5+). If the constraint is "the object and arrays were produced by the runtime and checked via an internal slot", then there is no way for a transpiler to meet that constraint.

The constraint would be "frozen", not "internal slot".

Could u add comment here to explain the worst case if we use "only cache frozen" so everyone could understand the situation

The issue is silently doing the wrong behavior. Tagged template literals are expected to receive a consistent object identity, and using String.dedent must not break that expectation. Any kind of behavior could be built upon the expectation that "I'll only do this once per tagged template".

The cases that I'm most familiar with is for DOM rendering. With this, the most obvious negative side-effect is that the performance will be bad (the DOM would be wiped out because we're rendering a "new" template). But it also effects user state (text written in an input, focus state, etc). Silently doing incorrect behaviors that look like they're working correctly are the worst kinds of bugs to fix, because the bug isn't obvious.

At least with the current cache everything behavior, the bug is obvious. You'll see some incorrect value when you render, and it'll be possible to track the bug back to String.dedent. Maybe the fix won't be obvious (don't mutate the array), but the there'll be a known place to start searching.

[brad4d]

If I were using String.dedent with a mutable array, I would definitely expect it to use the values the array currently has. The caching is an implementation detail I likely wouldn't even know about.

If I get a thrown exception saying the array must be frozen, I immediately know how to deal with that.
If I get incorrect output only sometimes (when the array has been modified, which likely doesn't always happen), it may take a long time for me to notice the problem and even longer to figure out why. And, when I figure out why, I will likely think that String.dedent is misbehaving.

So, I believe throwing an exception for an unfrozen array is the most user friendly choice.

Well, actually, I think just letting it be slow (no caching) for an unfrozen array is the most user friendly, but it seems others disagree on that point.