Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InterpolatedStringHandlerArgumentAttribute("") is not correctly taken into account when the receiver is a struct #58514

Closed
shkdee opened this issue Dec 28, 2021 · 0 comments · Fixed by #58764 or #58657
Assignees
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers Bug Feature - Interpolated String Improvements Interpolated string improvements
Milestone

Comments

@shkdee
Copy link

shkdee commented Dec 28, 2021

Version Used:
Visual Studio v17.0.4

SDK .NET (reflétant tous les fichiers global.json) :
 Version:   6.0.101
 Commit:    ef49f6213a

Environnement d'exécution :
 OS Name:     Windows
 OS Version:  10.0.19044
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\6.0.101\

Host (useful for support):
  Version: 6.0.1
  Commit:  3a25a7f1cc

Steps to Reproduce:

The issue occurs when a you define a method on struct, that expects to receive an InterpolatedStringHandler, and when you specify [InterpolatedStringHandlerArgument("")] for the arguments:

internal struct StructLogger
{
    ....
    public void Log([InterpolatedStringHandlerArgument("")] DummyHandler handler) => ....;
    ....
}

[InterpolatedStringHandler]
internal ref struct DummyHandler
{
    ...
    public DummyHandler(int literalLength, int formattedCount, StructLogger logger, out bool enabled)
    {
        // here, logger contains either the default value for StructLogger, or possibly another value than the expected one
    }
}

Basically, depending on how you call StructLogger.Log and on how you compiled the code (!), the value logger in the DummyHandler constructor can be either the default value for StructLogger, or a previously created value, but not the one on which you actually called Log($"..."). I believe this is a compiler issue, because of the behavior difference between debug and release mode, and because the issue clearly looks to be present in the IL code when you decompile the generated dll (but I am less familiar with that so I won't try to point it out directly).

I wrote a test case to demonstrate the issue, you just have to compile the code as a single file console project and run it. It is enclosed at the bottom of this message. It is also interesting to note that the behavior is not the same in debug and release.

Expected Behavior:
Output the following:

Start
Creating DummyHandler from StructLogger#1
StructLogger#1:
(1) i=0
Creating DummyHandler from StructLogger#2
StructLogger#2:
(2) i=0
Creating DummyHandler from ClassLogger#3
ClassLogger#3:
(3) i=0
Creating DummyHandler from ClassLogger#4
ClassLogger#4:
(4) i=0
End

Actual Behavior:
In debug, you get:

Start
Creating DummyHandler from StructLogger#0
StructLogger#1: log:0
(1) i=1
Creating DummyHandler from StructLogger#0
StructLogger#2: log:1
(2) i=2
Creating DummyHandler from ClassLogger#3
ClassLogger#3:
(3) i=2
Creating DummyHandler from ClassLogger#4
ClassLogger#4:
(4) i=2
End

And in release, you get (note the discrepancy between the logger id passed to the DummyHandler constructor and the one actually used to issue the log line):

Start
Creating DummyHandler from StructLogger#0
StructLogger#1: log:0
(1) i=1
Creating DummyHandler from StructLogger#1
StructLogger#2:
(2) i=1
Creating DummyHandler from ClassLogger#3
ClassLogger#3:
(3) i=1
Creating DummyHandler from ClassLogger#4
ClassLogger#4:
(4) i=1
End

Test case file content

using System.Runtime.CompilerServices;
using System.Text;

Console.WriteLine("Start");
var i = 0;
new StructLogger(true, 1).Log($"log:{i++}");
Console.WriteLine($"(1) i={i}");
var s = new StructLogger(true, 2);
s.Log($"log:{i++}");
Console.WriteLine($"(2) i={i}");
new ClassLogger(true, 3).Log($"log:{i++}");
Console.WriteLine($"(3) i={i}");
var c = new ClassLogger(true, 4);
c.Log($"log:{i++}");
Console.WriteLine($"(4) i={i}");
Console.WriteLine("End");

internal readonly struct StructLogger
{
    private readonly bool _disabled;
    private readonly int _id;

    public bool Disabled => _disabled;
    public int Id => _id;

    public StructLogger(bool disabled, int id)
    {
        _disabled = disabled;
        _id = id;
    }

    public void Log([InterpolatedStringHandlerArgument("")] DummyHandler handler) => Console.WriteLine($"StructLogger#{_id}: " + handler.GetContent());
}

// Include a class-based logger to show what should happen
internal class ClassLogger
{
    public bool Disabled { get; }
    public int Id { get; }

    public ClassLogger(bool disabled, int id)
    {
        Disabled = disabled;
        Id = id;
    }

    public void Log([InterpolatedStringHandlerArgument("")] DummyHandler handler) => Console.WriteLine($"ClassLogger#{Id}: " + handler.GetContent());
}

[InterpolatedStringHandler]
internal ref struct DummyHandler
{
    private readonly StringBuilder? _builder;
    public DummyHandler(int literalLength, int formattedCount, StructLogger structLogger, out bool enabled)
    {
        Console.WriteLine($"Creating DummyHandler from StructLogger#{structLogger.Id}");
        enabled = !structLogger.Disabled;
        _builder = structLogger.Disabled ? null : new StringBuilder();
    }
    public DummyHandler(int literalLength, int formattedCount, ClassLogger classLogger, out bool enabled)
    {
        Console.WriteLine($"Creating DummyHandler from ClassLogger#{classLogger.Id}");
        enabled = !classLogger.Disabled;
        _builder = classLogger.Disabled ? null : new StringBuilder();
    }

    public string? GetContent() => _builder?.ToString();

    public void AppendLiteral(string s) => _builder?.Append(s);
    public void AppendFormatted<T>(T t) => _builder?.Append(t);
}
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 28, 2021
@jaredpar jaredpar added Bug Feature - Const Interpolated Strings and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 3, 2022
@jaredpar jaredpar added this to the 17.1 milestone Jan 3, 2022
333fred added a commit to 333fred/roslyn that referenced this issue Jan 7, 2022
…ted string handlers

The SpillSequenceSpiller has an assumption that, unless a BoundSequence contains an await, it will not affect any BoundSpillSequence nodes. This isn't true for interpolated strings, as the receiver could be a sequence that spills the receiver to a temp later used by the handler constructor. We therefore now mark such BoundSequences as needing to be spilled, and in such cases the SpillSequenceSpiller will treat the sequence as if it needs to be spilled, regardless of whether it contained an `await` or not.

Fixes dotnet#58514.
333fred added a commit to 333fred/roslyn that referenced this issue Jan 7, 2022
…ted string handlers

The SpillSequenceSpiller has an assumption that, unless a BoundSequence contains an await, it will not affect any BoundSpillSequence nodes. This isn't true for interpolated strings, as the receiver could be a sequence that spills the receiver to a temp later used by the handler constructor. We therefore now mark such BoundSequences as needing to be spilled, and in such cases the SpillSequenceSpiller will treat the sequence as if it needs to be spilled, regardless of whether it contained an `await` or not.

Fixes dotnet#58514.
@333fred 333fred added the 4 - In Review A fix for the issue is submitted for review. label Jan 7, 2022
333fred added a commit to 333fred/roslyn that referenced this issue Jan 11, 2022
dotnet#58657 was also a fix for 58514, by forcing the compiler to spill sequences in this scenario as well. This just adds tests to verify the scenario is addressed.

Fixes dotnet#58514.
333fred added a commit that referenced this issue Jan 20, 2022
#58657 was also a fix for 58514, by forcing the compiler to spill sequences in this scenario as well. This just adds tests to verify the scenario is addressed.

Fixes #58514.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers Bug Feature - Interpolated String Improvements Interpolated string improvements
Projects
None yet
3 participants