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

Nullable value type receiver of a constrained call is unexpectedly copied #66162

Closed
AlekseyTs opened this issue Dec 30, 2022 · 2 comments · Fixed by #66311
Closed

Nullable value type receiver of a constrained call is unexpectedly copied #66162

AlekseyTs opened this issue Dec 30, 2022 · 2 comments · Fixed by #66311
Assignees
Labels
Area-Compilers Bug Regression Resolution-Won't Fix A real bug, but Triage feels that the issue is not impactful enough to spend time on.
Milestone

Comments

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Dec 30, 2022

        [Fact]
        public void GenericTypeParameterAsReceiver_Call_Nullable()
        {
            var source = @"
using System;

#pragma warning disable CS0659 // 'Item' overrides Object.Equals(object o) but does not override Object.GetHashCode()
    
struct Item
{
    public int Count;

    public override bool Equals(object obj)
    {
        Console.WriteLine(""Position Equals for item '{0}'"", Count);
        return base.Equals(obj);
    }
}

class Program
{
    static void Main()
    {
        Item? item1 = new Item {Count = 1};
        Call1(item1);
        Item? item2 = new Item {Count = 2};
        Call2(ref item2);
    }

    static void Call1<T>(T item)
    {
        item.Equals(GetOffset(ref item));
    }

    static void Call2<T>(ref T item)
    {
        item.Equals(GetOffset(ref item));
    }
    
    static int value = 0;
    static int GetOffset<T>(ref T item)
    {
        item = (T)(object)new Item {Count = --value};
        return 0;
    }
}
";
            var verifier = CompileAndVerify(source, options: TestOptions.ReleaseExe, expectedOutput: @"
Position Equals for item '-1'
Position Equals for item '-2'
").VerifyDiagnostics();
        }

Observed:

Microsoft.CodeAnalysis.CSharp.UnitTests.CodeGen.CodeGenCallTests.GenericTypeParameterAsReceiver_Call_Nullable [FAIL]
  Roslyn.Test.Utilities.ExecutionException : 
  Execution failed for assembly 'd9d40dcd-4b54-4258-8425-bf38af6ab1e7, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'.
  Expected: 
  Position Equals for item '-1'
  Position Equals for item '-2'
  
  Actual: Position Equals for item '1'
  Position Equals for item '2'

This is a regression from #65642

@AlekseyTs AlekseyTs self-assigned this Dec 30, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Dec 30, 2022
@jcouv jcouv added this to the C# 12.0 milestone Dec 31, 2022
@jcouv jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Dec 31, 2022
@jaredpar jaredpar modified the milestones: C# 12.0, 17.4 P4, 17.5 Jan 5, 2023
AlekseyTs added a commit to AlekseyTs/roslyn that referenced this issue Jan 8, 2023
The check that we used to detect value types at runtime didn’t quite work for
nullable value types due to special boxing behavior for them.
Switching to `typeof(T).IsValueType` check instead.

Fixes dotnet#66162.
Related to dotnet#63221.
AlekseyTs added a commit that referenced this issue Jan 10, 2023
The check that we used to detect value types at runtime didn’t quite work for
nullable value types due to special boxing behavior for them.
Switching to `typeof(T).IsValueType` check instead.

Fixes #66162.
Related to #63221.
@AlekseyTs
Copy link
Contributor Author

Reverting the fix due to dotnet/runtime#80429 (comment)

@AlekseyTs AlekseyTs reopened this Jan 13, 2023
@AlekseyTs
Copy link
Contributor Author

The scenario that is affected is very restricted:

  • The unexpected copy is created only for an invocation of a virtual method declared by System.Object or by System.ValueType.
  • The unexpected copy is created only if the method invocation has non-trivial argument.
  • The fact that the copy is taken can be observed only if the method is overridden by System.Nullable<T> (otherwise the receiver is going to be boxed and copied anyway).

At the moment, System.Nullable<T> overrides ToString, GetHashCode and Equals. Only Equals meets all the requirements. So, only invocations of Equals could potentially be performed on a receiver that doesn't have mutations that the argument expression performs. Only for invocation of Equals mutations performed by Equals of the underlying type potentially are not be observable by the caller in the receiver of System.Nullable<T> type.

Given this, we are comfortable with not fixing this regression for now. We might revisit this decision once a code gen strategy with acceptable performance characteristics becomes available.

@AlekseyTs AlekseyTs added the Resolution-Won't Fix A real bug, but Triage feels that the issue is not impactful enough to spend time on. label Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Bug Regression Resolution-Won't Fix A real bug, but Triage feels that the issue is not impactful enough to spend time on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants