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.

[jridgewell]

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.

This breaks with the expectation of every tag function I've seen. Caching is done cheaply on the array identity using a WeakMap, and if you modify it, you're gonna still get the cached results. I don't think String.dedent is special here.

[bakkot]

Among

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

My preference ordering is 2>1>3>>4, but the difference between 1 and 3 is not large for me (and anything except 4 would be acceptable). The downside of 3 is just that when using it manually you have to Object.freeze(), which is a little annoying but at least you'll get an error.

[hax]

The reason I don't like "always cache" is String.raw do not have such behavior.

About DOM rendering, I suppose we are talking code like:

String.dedent(html)`
   <div>...</div>
`

The point here is html tag functions rely on frozen semantic, and we are worrying Babel loose mode compile the code like String.dedent({raw: ["<div>...</div>", ""]}), not String.dedent({raw: Object.freeze(["<div>...</div>", ""])})

Personally I feel this is the edge case, and mainly a toolchain issue. So can we have a compromised solution like:

5. use 2 (only cache frozen) for String.dedent(array, ...subs), use 3 or 1 semantic for String.dedent(func) ?

[jridgewell]

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

My ordering here would be 1=3>2 (and I will reject 4 entirely)

[gibson042]

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.

@jridgewell Except if you don't see the incorrect value because you overlooked it. I would definitely include propagating array mutation at a distance in the category of "silently doing incorrect behaviors that look like they're working correctly".

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.

@brad4d Exactly. 👍

[bakkot]

So, while it's not my first choice, it sounds like "cache frozen, throw if not frozen" is acceptable to everyone?

[hax]

This breaks with the expectation of every tag function I've seen.

@jridgewell To be honest, I don't see many tag function (except DOM parsing) expect that (and do cache), for example, it seems no userland dedent util functions (which mentioned in the readme) do cache. 😅

[jridgewell]

The reason I don't like "always cache" is String.raw do not have such behavior.

String.raw and String.cooked don't perform any caching. Not doing it doesn't matter much, since they're the final step (they don't wrap other tag functions). But String.dedent needs caching behavior, because the tags we wrap will expect it.

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.

Exactly

String.dedent won't be the only tag function that breaks with this, every one that does caching uses weakmap.get(array) to do this. I think this is such a niche edge-case, I've never seen anyone try to modify a template strings arrray.

So, while it's not my first choice, it sounds like "cache frozen, throw if not frozen" is acceptable to everyone?

That would be ok with me.

for example, it seems no userland dedent util functions (which mentioned in the readme) do cache.

No dedent function wraps other tag functions. We need caching because those wrapped functions will expect consistent object identity.

[hax]

Update options:

1. always cache (current proposal)
2. only cache frozen
3. cache frozen, throw if not frozen
4. no cache
5. use 2 or 4 (should have no observable diff) for String.dedent(obj, ...subs), use 3 or 1 semantic for String.dedent(func)

I add the extra option 5, because normally direct usage of String.dedent do not have big concern, only wrapped tag functions have such concern.

[ljharb]

fwiw my preference is 2, 1, 3, 4. caching is a performance optimization, and "not caching" isn't breakage, it's just slowdown - if it is breakage, it's not actually a cache, it's something else.

[bakkot]

@hax I don't really like your option 5 because it means that String.dedent(Sting.cooked)(obj) would be different from String.dedent(obj). I'd prefer to do the same thing in both cases, I think; the benefits of making them different are less than the costs of making it more complicated.

[hax]

String.raw and String.cooked don't perform any caching. Not doing it doesn't matter much, since they're the final step (they don't wrap other tag functions). But String.dedent needs caching behavior, because the tags we wrap will expect it.

I think this is why I feel uncomfortable with "always cache", because String.dedent also could be the final step (just like the example in the slide). And if it's the final step, "caching" is just a implementation details.

[hax]

@bakkot Yeah, the option 5 is a trade-off so it lose some nature. But I think it still a good option.

There is no real harm that String.dedent(tag) not behave same as tag (because String.dedent(tag) give u a wrapped func, it supposed to modify behavior). On the other side, option 1 and 3 really introduce different behavior between String.dedent and String.raw on unfrozen obj.

[jridgewell]

Can we agree then on the throwing solution?

[Josh-Cena]

What's the benefit of throwing compared to not caching at all? If I pass a custom object that pretends to be a template, I wouldn't really expect anything to be cached anyway (even with a wrapped tag). This just makes dedenting dynamic strings harder: if I can't do String.dedent(str), I can live with String.dedent({ raw: [str] }), but now I have to do String.dedent({ raw: Object.freeze([str]) }).

[jridgewell]

If I pass a custom object that pretends to be a template, I wouldn't really expect anything to be cached anyway (even with a wrapped tag).

I think #75 (comment) answers that. Silently doing something expensive when we know it's incorrect is extremely difficult to debug. At least loudly throwing will help the dev fix the issue.

[Josh-Cena]

The expectation, IMO, is that template literals are cached and not recomputed. Template literals mean that the string literals always have stable values, and these stable values are trivially inlined in the source code, so the mental burden is really low. This is not the case with an ad hoc object. It's almost the exact opposite semantic—the template object { raw: [str] } is expected to change, either because I used a different value as str, or because I mutated the array.

My concern is "help fix the issue" where the issue is really a non-issue—if I go to the trouble of creating an ad hoc object just to trick String.dedent, why would I expect it to magically cache anything anyway? I would strongly prefer the non-wrapping version to not throw at the minimum; but even for the wrapping version, unless there's some userland library that also throws for non-frozen template objects, I don't think we are preventing anything that's not possible under the status quo.

[jridgewell]

It's common for transformers to turn tagged template literals into unfrozen arrays, so it's not really possible to determine what's literal source code or dynamically generated arrays.

I think the expectation that anyone is going to mutate an array and call a tag function is really the non-issue, though. Who does that? Again, every tag function that I've seen that does caching, does it by using the array's identity. This is just the normal practice.

[ljharb]

What's incorrect about doing something expensive, if it produces the same result?

[jridgewell]

Lit operates a bit like container.innerHTML = someHTML. When rendering the same template, the diffing is fast, yes. But it also preserves user state that cannot be represented in HTML, like focus state, input values, toggle state. It's not just performance, and it'll be difficult to isolate.

[ljharb]

ah ok, so you're using the identity of the template array to distinguish individual components, and thus individual bags of state?

[jridgewell]

The state is maintained by the browser itself, we're using the array's identity to determine if this is a new template. If it's the same template, we can just do basic DOM diffing on the dynamic expressions in the template. If it's a new template, the old tree is completely removed and the new tree is inserted. Not maintaining the array's identity causes lit to completely wipe out the DOM state during every render.

[Josh-Cena]

So, IIUC, there's only an observable difference in the case where (a) you are passing a dynamic object (b) you are wrapping another tag. In this case we need to compare it against the world where a dynamic object is directly passed to the said tag—and by mimicking their behavior, we would introduce no degradation of experience, to say the least. If they always cache, it means mutating the array is not going to work anyway, and we are fine to also cache. And I don't really think they throw for non-frozen objects, do they?

[jridgewell]

Every tag I've seen uses a WeakMap on the array identity, if they cache. I've never seen one throw for an unfrozen array.

[ljharb]

I wonder if things like Lit could do a check on the array like "if the array is frozen, use its identity, else serialize a shallow comparison to a memoized identity"? or would that be prohibitive in userland

[Josh-Cena]

I prefer non-wrapping dedent calls to not throw for non-frozen objects (and probably not cache either, so that mutations work as expected), but I don't have strong opinions about wrapping calls. This makes me look at option 5.

String.dedent(String.cooked)(obj) would be different from String.dedent(obj).

This is the only argument I see against option 5 (apologies if I missed any). But IMO throwing an error for the former case does not constitute an unexpected behavior difference—"throws if called with another tag and then passed with a non-frozen object" sounds like a reasonable invariant.

[jridgewell]

or would that be prohibitive in userland

String equality on large strings gets to be kinda expensive. Again, this is a super edge case, it's not expected that you ever mutate the input template strings array.

[jridgewell]

I check the various transformers:

• TS UNFROZEN
• Babel loose UNFROZEN
• Babel default frozen
• esbuild frozen
• SWC loose UNFROZEN
• SWC default frozen

It's the TS one that worries me. I don't even know how to configure it to output a frozen template strings array. I really think we should continue with either the current semantics, or go the throwing semantics. But the uncached semantics seems like the wrong choice.

[rbuckton]

I'm sure we can change TS to freeze the generated template object if that's necessary.

[ljharb]

That should likely be done anyways, I’d hope.

[jridgewell]

Yah, I agree that TS should update to freeze, but it also requires devs to update to get the correct behavior.

[rbuckton]

Yah, I agree that TS should update to freeze, but it also requires devs to update to get the correct behavior.

It depends. We could implement the freeze-behavior in the __makeTemplateObject helper. Anyone using --importHelpers could potentially get this for free if we patch tslib. Anyone not using --importHelpers would still need to rebuild, however.