elide ${} after they're no longer needed and consistently coerce interpolations in untagged string expressions to string#392
Conversation
Partially addresses #388 `.toString()` inference in complex string expressions is still needed. Signed-off-by: Mike Samuel <mikesamuel@gmail.com>
| input = $$""" | ||
| |$${"\"\"\""} | ||
| |:for (var i = 1; i < 100; i *= 2) { | ||
| | ~${i.toString()}, ${} |
There was a problem hiding this comment.
Working on a commit so the .toString() is implied.
tjpalmer
left a comment
tjpalmer
left a comment
There was a problem hiding this comment.
I know this is marked as draft but looks good overall, so feel free to merge if you're happy with it as is.
Previously, *StrCatMacro* was a macro that resolved to *StrCatFn*, a regular, translatable function that concatenates string values. *StrCatMacro* took care to implicitly coerce its arguments to strings. - `null` values coerce to `"null"` - otherwise, it's equivalent to structurally applying `.toString()` to the argument, but if the type is *String* we can skip that step. This multi-step logic, and the need to generate `if (isNull(...))` for nullable values, involved some code generation. Also, with the addition of embedded statements to string expressions, not all untagged string expressions go through *StrCatMacro*. This commit extracts out the logic for string coercions and embeds it in a helper macro, *str*, which erases itself once it has a type for its argument to code that does the steps above. This commit also reworks untagged string expression handling so that interpolated expressions are consistently coerced to strings. Signed-off-by: Mike Samuel <mikesamuel@gmail.com>
mikesamuel
changed the title
${} after they're no longer needed${} after they're no longer needed and consistently coerce interpolations in untagged string expressions to string
@tjpalmer I added another commit, so I think the bug is fixed now. |
| if (isTagged) { | ||
| Replant(next) | ||
| } else { | ||
| Call(next.pos, CoerceToString) { |
There was a problem hiding this comment.
This fixes the bug. Since we weren't going through StrCatMacro, no coercion was being done for string expressions that have embedded statements and are untagged.
| Call(macroEnv.pos) { | ||
| V(macroEnv.callee.pos, BuiltinFuns.vStrCatFn) | ||
| for ((arg, argType) in argsWithTypes) { | ||
| if (argType == WellKnownTypes.stringType) { |
There was a problem hiding this comment.
This logic is pulled out into a helper below. The only changes are that, instead of looking at macroEnv for information available from the document, we just get that from arg directly.
| } | ||
| } | ||
|
|
||
| internal object CoerceToString : SpecialFunction, BuiltinMacro("str", null) { |
There was a problem hiding this comment.
str is short and reminiscent of Python's string coercion operator.
There was a problem hiding this comment.
Since str waits for type stage, it actually erases at the stage after Stage.Type, Stage.FunctionMacro
|
I looked up the be-js errors and see these messages: |
tjpalmer
left a comment
tjpalmer
left a comment
There was a problem hiding this comment.
I think the funtest fails are real.
Some expansions, type-aware `as` and `is` simplification and constant inlining were preventing the string coercion macro from getting type information. Previously, doing manual post-Typer simplification of `.toString()` worked because it happened at a very specific place. Having type aware macros work is generally good, so I rolled some changes in to preserve type info better. Now, `as` and `is` desugar to `if` explicitly which means they flow into the Weaver nicely. And subtree inlining preserves type information. Signed-off-by: Mike Samuel <mikesamuel@gmail.com>
I think these are fixed now. |
| |{ | ||
| | run: ["1, 2, 4, 8, 16, 32, 64, and so on", "String"], | ||
| |} | ||
| """.trimMargin(), |
Signed-off-by: Mike Samuel <mikesamuel@gmail.com>
|
Looks like a timeout: java.util.concurrent.TimeoutException: functionsRestFormal() timed out after 60 seconds |
2 participants
Fixes #388