dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.89k stars 4.01k forks source link

`nameof(X)` in Interpolated String Handlers are not always lowered to constants #67295

Open colejohnson66 opened 1 year ago

colejohnson66 commented 1 year ago

Version Used: sharplab.io; main (24 Jan 2023)

Steps to Reproduce:

  1. Use an interpolated string with a constant string and a nameof expression.
  2. Observe the whole string lowered to a constant.
  3. Change the interpolated string to add a property.
  4. Observe the nameof expression is now lowered to AppendFormatted (not AppendLiteral)

Source:

public int Property => 2;
public void Test() {
    Console.WriteLine($"abc {nameof(Test)}");
    Console.WriteLine($"abc {nameof(Test)} {Property}");
}

Expected Behavior:

public void Test()
{
    Console.WriteLine("abc Test");
    DefaultInterpolatedStringHandler defaultInterpolatedStringHandler = new DefaultInterpolatedStringHandler(9, 1);
    defaultInterpolatedStringHandler.AppendLiteral("abc Test "); // <-- merged
    defaultInterpolatedStringHandler.AppendFormatted(Property);
    Console.WriteLine(defaultInterpolatedStringHandler.ToStringAndClear());
}

Actual Behavior:

public void Test()
{
    Console.WriteLine("abc Test");
    DefaultInterpolatedStringHandler defaultInterpolatedStringHandler = new DefaultInterpolatedStringHandler(5, 2);
    defaultInterpolatedStringHandler.AppendLiteral("abc ");
    defaultInterpolatedStringHandler.AppendFormatted("Test"); // <-- not merged
    defaultInterpolatedStringHandler.AppendLiteral(" ");
    defaultInterpolatedStringHandler.AppendFormatted(Property);
    Console.WriteLine(defaultInterpolatedStringHandler.ToStringAndClear());
}

Note how the nameof expression was combined with the literal string next to it in the first, but not in the second. In addition, the second one lowers the nameof into an AppendFormatted call instead of an AppendLiteral.

Performance

public class InterpolatedBenchmarks
{
#pragma warning disable CA1822
    public int Property => 2;
#pragma warning restore CA1822

    [Benchmark(Baseline = true)]
    public string TestA() =>
        "test TestB " + Property;

    [Benchmark]
    public string TestB() =>
        $"test TestB {Property}";

    [Benchmark]
    public string TestC() =>
        $"test {nameof(TestB)} {Property}";
}

Running the above benchmark (with BenchmarkDotNet) on 7.0.323.6910 (x64 RyuJIT AVX2) shows a slight performance decrease for the actual compiler output compared to what I would expect:

Method Mean Error StdDev Ratio RatioSD
TestA 14.84 ns 0.343 ns 0.321 ns 1.00 0.00
TestB 36.03 ns 0.702 ns 0.656 ns 2.43 0.08
TestC 46.31 ns 0.516 ns 0.458 ns 3.12 0.08

In fact, performance is horrible compared to plain concatenation (lowered to string.Concat).

jcouv commented 1 year ago

Tagging @333fred to advise/triage.

333fred commented 1 year ago

We could certainly make this smarter, but it's not going to be trivial due to how we do (or don't do, in this case) constant folding when the entire string isn't a constant value.

333fred commented 1 year ago

We'd review a pr that implements this (likely would be a change in how the local rewriter lowers these), but are unlikely to prioritize it ourselves.

333fred commented 1 year ago

Oh, and one more detail before I forget: AppendFormatted calls can have other important side-effects, like CallerExpression attributes, or being used for logging (where the fact that this is a placeholder is important). We can only optimize this for regular strings because of this.

colejohnson66 commented 1 year ago

If allowing those CallerX attributes is so important, why isn't the constant+nameof one lowered to a single AppendLiteral call instead of AppendLiteral("abc ") / AppendX("Test")? Basically, which is more "correct"? Should nameof expressions always be a seperate call to AppendX (in which the second line is correct)? Or should they always be treated as a string constant and merged (in which the first line is correct)? If the former, is AppendFormatted correct, or should that be AppendLiteral?

Because my example above shows both things happening, but the choice is dependent on using a property (or possibly other things); the nameof expression is treated as a literal for the first call (what would be AppendLiteral) and merges it with the string prior, but it's treated as an interpolated value for the second (AppendFormatted).

333fred commented 1 year ago

If allowing those CallerX attributes is so important, why isn't the constant+nameof one lowered to a single AppendLiteral call instead of AppendLiteral("abc ") / AppendX("Test")?

We didn't feel that these were important for AppendLiteral, and didn't spec it this way. AppendLiteral is specified as being looked up once for the entire string, and using that one result for every literal component. AppendFormatted is not specified this way, as every interpolation hole must be individually looked up.

colejohnson66 commented 1 year ago

By "lookup", I assume you mean name resolution? In which case, that's fine. But that doesn't explain why nameof(Test) is lowered as a constant in the $"abc {nameof(Test)}" case, but lowered to a "hole" in the $"abc {nameof(Test)}{Property}" case.

My comment was pointing out this discrepancy and asking "which is ideal?" Because, if we need to take CallerX attributes into account, then the "hole" in the $"abc {nameof(Test)}" case should be lowered to an AppendFormatted call, but it's not.

333fred commented 1 year ago

These questions are all spelled out in the interpolated strings draft spec. When the string is a constant, we always prefer overloads that take strings. When the string is not a constant (has holes that are not constants), we prefer overloads that take interpolated string handlers. For converting an interpolated string to a string, the compiler is free to optimize as it chooses, including just emitting a string constant, using Concat/Format, or using the DefaultInterpolatedStringHandler. For converting an interpolated string to an observable handler type, the compiler is not free to make optimizations that would affect which Append methods is called.

tats-u commented 2 months ago

Not only nameof but also all string constants:

71801 (Issue)

72308 (PR)

I've approached only built-in handlers (DefaultInterpolatedStringHandler and so on) for the time being.