feat: Report a diagnostic when attempting to use a template literal

[saulecabrera]

Fixes #1615

I think it makes sense to report a diagnostic until #466 is implemented. We discovered this by accident as it was causing an unexpected behavior in one of our sdks.

• I've read the contributing guidelines

[MaxGraey]

I guess better report only if string contain interpolations like

const str = `hi ${name}`;

But back-ticked string literals works normally without ${} and usually using for multiline text without explicit new lines.

[saulecabrera]

I guess better report only if string contain interpolations like

const str = `hi ${name}`;

But back-ticked string literals works normally without ${} and usually using for multiline text without explicit new lines.

I'd argue that providing multiline strings through template literals is a half-baked feature. We should either support them fully or not support them at all. This is mostly based on the spec https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals#multi-line_strings

Any newline characters inserted in the source are part of the template literal.

If the plan is to keep the current behavior, I think that it should be at least well documented. I'd be happy to update to the documentation if you don't agree with this change.

[saulecabrera]

if string contain interpolations like

Also, if your suggestion is to detect interpolation, it would be better to actually implement the interpolation feature itself, doing so really voids the objective of this change.

[saulecabrera]

Thoughts @dcodeIO?

[dcodeIO]

I am fine with emitting a diagnostic here to avoid unexpected outcomes for the time being. Wondering if the diagnostic can be raised only if such a literal becomes actually compiled, though, as it seems that it round-trips from source to AST and back from AST to source just fine already. But I may be wrong :)

[saulecabrera]

@dcodeIO Thinking a bit more about this, I'm not sure if an error diagnostic is the right approach since it will require a breaking change for a feature that will be eventually implemented, on top of that, multiline strings already work correctly. I'm thinking that a warning diagnostic is better? Thoughts? I'm just thinking that I can't justify a breaking change for this, I just want something that is enough to notify the developer that interpolation is not implemented and therefore should not be used.

Wondering if the diagnostic can be raised only if such a literal becomes actually compiled, though,

According to my understanding, to make this work, we would need to read the string including the quotes (or some other metadata that indicates which type of string we are compiling), so that we can know when to actually report the diagnostic. Right now we don't store that (at least it's not obvious); I obviously know less than you do in terms of options, but doing so seems to be quite an overhead for a diagnostic that can be caught early.

[dcodeIO]

Hmm, right, that information is not present anymore in the AST. In this case I'm fine with the warning :)

[saulecabrera]

Sounds good. I changed the error to a warning.

[saulecabrera]

@dcodeIO would it be possible for you to review this? Or is it better to wait until you finish updating binaryen?

[dcodeIO]

Have been thinking about this a bit more, and I am still a bit concerned that someone else may be unhappy with the warning when only using template literals for multi line strings, as it cannot be silenced, and didn't produce a warning so far. I don't feel strongly, though, as this case seems rare enough, hence approving, yet there is a chance that we'll have to touch this again if our assumptions are wrong. Would you be fine with looking into that as well, if it happens?

[saulecabrera]

Have been thinking about this a bit more, and I am still a bit concerned that someone else may be unhappy with the warning when only using template literals for multi line strings, as it cannot be silenced, and didn't produce a warning so far. I don't feel strongly, though, as this case seems rare enough, hence approving, yet there is a chance that we'll have to touch this again if our assumptions are wrong. Would you be fine with looking into that as well, if it happens?

That totally makes sense, I agree that this solution is mostly a bandaid, unfortunately, mostly to make the developers aware that there are going to be unexpected behaviors if they attempt to use interpolation. And yeah I can look into it if it happens again, feel free to ping me and I'll be glad to help.

A good green path forward (after merging this) would be to start (progressively) working on the actual interpolation feature. I see that there's a stale / draft PR here https://github.com/AssemblyScript/assemblyscript/pull/1427/files (two actually). If you agree I can start progressively working on the feature, consolidating the feedback from both PRs. Before doing so, I'd like to ask, given that this is a new language feature, from the top of your head, does this feature has any implications with any of the runtime work that you're doing? I'm mostly asking because if that is the case then the wisest thing to do is to wait I guess.

[dcodeIO]

What we may want there eventually is to split template literals into an array of string parts present in static memory during compilation, so the actual implementation can be as simple as converting all the parameters to strings, compute the overall length of all string parts plus the converted parameters, and build a single string without having to resort to multiple concatenations or something. I don't think that it would interfere, and I'd be happy to see that landed :)

Shall we merge this one, hope for the best and continue working towards it?

[saulecabrera]

Shall we merge this one, hope for the best and continue working towards it?

Yeah, I agree with this 👍