Optimize interpolated string with no holes

[kerams]

let stringOnly () = $"no hole" 
let printed () = printf $"printed no hole"

becomes

public static string stringOnly()
{
    return "no hole";
}

public static void printed()
{
    PrintfFormat<Unit, TextWriter, Unit, Unit> format = new PrintfFormat<Unit, TextWriter, Unit, Unit, Unit>("printed no hole");
    PrintfModule.PrintFormatToTextWriter(Console.Out, format);
}

instead of

public static string stringOnly()
{
    return PrintfModule.PrintFormatToStringThen(new PrintfFormat<string, Unit, string, string, Unit>("no hole", new object[0], null));
}

public static void printed()
{
    PrintfFormat<Unit, TextWriter, Unit, Unit> format = new PrintfFormat<Unit, TextWriter, Unit, Unit, Unit>("printed no hole", new object[0], null);
    PrintfModule.PrintFormatToTextWriter(Console.Out, format);
}

[kerams]

If I were to create a reduction of sprintf "xxx" into "xxx", where would be the place to do it? I don't see how in CheckExpressions, so then I considered OptimizeApplication, checking for sprintf and 'constant' format, but it seems incredibly specific compared to the what the function does.

[charlesroddie]

$"no hole" is terrible code. Instead of making it perform better, adding a 10s delay would be an appropriate solution, a fair punishment for writing this.

An analyzer is the correct solution isn't it?

[kerams]

Yes, it's pointless and no one sets out to write that initially, but I can imagine a scenario where there was actual interpolation, then someone removed it, but forgot the $. The same with sprintf "xxx", perhaps less likely.

[charlesroddie]

Yes, and the right solution is to help fix the code (using analyzers) rather than work around it. Isn't it?

[kerams]

I'd say a compiler/analyzer warning would be complementary, and concerned with code style rather than performance. There's no reason we should emit suboptimal code if the user does not notice or heed the warning (when it's this easy to fix, to be precise). If you consider one extra character a crime against humanity, by all means go ahead and also implement an analyzer. I'm not fussed myself.

[cartermp]

I think this is a fine addition. We're not in the business of punishing people for writing what someone might personally find to be bad code.

[dsyme]

We're not in the business of punishing people for writing what someone might personally find to be bad code.

:) let mutable x = ... 🤣

[dsyme]

In theory this changes the quotation form for an interpolated string. I don't mind that change, however could you add a test in tests/fsharp/core/quotes/test.fsx or elsewhere to test for that and pin down the new form please?

e.g. check results of <@ $"abc" @> and you may as well add a test for <@ $"abc {1} def" @> while you're at it.

[dsyme]

Approved, just add a test for quotation forms please

[kerams]

Done. Will the test run automatically or do I need to wire it up somewhere?

[dsyme]

Done. Will the test run automatically or do I need to wire it up somewhere?

All of the initialization code of quotes/test.fsx runs automatically as one test

[dsyme]

Approved, subject to green ticks