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

Specially handle more scenarios for type parameters with 'allows ref struct' constraint #73232

Merged

Conversation

AlekseyTs
Copy link
Contributor

No description provided.

@AlekseyTs AlekseyTs requested a review from a team as a code owner April 25, 2024 13:49
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Apr 25, 2024
@AlekseyTs
Copy link
Contributor Author

@cston, @jjonescz, @dotnet/roslyn-compiler Please review.

1 similar comment
@AlekseyTs
Copy link
Contributor Author

@cston, @jjonescz, @dotnet/roslyn-compiler Please review.

@@ -3568,8 +3568,35 @@ private BoundExpression BindIsOperator(BinaryExpressionSyntax node, BindingDiagn

// * If either type is a restricted type, the type check isn't supported because
// a restricted type cannot be boxed or unboxed into.
Copy link
Member

@jjonescz jjonescz Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adjusting this comment to reflect the implementation changes. #Resolved

expectedOutput: ExecutionConditionUtil.IsMonoOrCoreClr ? "4" : null,
verify: ExecutionConditionUtil.IsMonoOrCoreClr ? Verification.Passes : Verification.Skipped).
VerifyDiagnostics(
// (6,13): warning CS0184: The given expression is never of the provided ('I1') type
Copy link
Member

@jjonescz jjonescz Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds confusing. S implements I1, so h2 is of type I1 in a sense, only the is operator cannot be applied to it. Should the diagnostic message be changed to reflect that? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds confusing. S implements I1, so h2 is of type I1 in a sense, only the is operator cannot be applied to it. Should the diagnostic message be changed to reflect that?

I would say this is a preexisting condition, which can be demonstrated with a scenario not involving interfaces:

warning CS0184: The given expression is never of the provided ('ValueType') type
if (h2 is System.ValueType) 

@@ -752,7 +752,6 @@ internal MethodSymbol TryFindDisposePatternMethod(BoundExpression expr, SyntaxNo
{
Debug.Assert(expr is object);
Debug.Assert(expr.Type is object);
// PROTOTYPE(RefStructInterfaces): adjust?
Debug.Assert(expr.Type.IsRefLikeType || hasAwait); // pattern dispose lookup is only valid on ref structs or asynchronous usings
Copy link
Member

@jjonescz jjonescz Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is pattern dispose not allowed on allows ref struct type parameters? Do we have tests for that? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is pattern dispose not allowed on allows ref struct type parameters? Do we have tests for that?

I think Using_06 covers that. The fact is also explicitly stated in https://github.com/dotnet/csharplang/blob/main/proposals/ref-struct-interfaces.md#using-statement section.

@AlekseyTs AlekseyTs requested a review from jjonescz April 29, 2024 15:45
@AlekseyTs
Copy link
Contributor Author

@cston, @jjonescz, @dotnet/roslyn-compiler Please review.

@@ -1872,7 +1872,7 @@ private BoundExpression SynthesizeMethodGroupReceiver(CSharpSyntaxNode syntax, A

private bool IsBadLocalOrParameterCapture(Symbol symbol, TypeSymbol type, RefKind refKind)
{
if (refKind != RefKind.None || type.IsRestrictedType()) // PROTOTYPE(RefStructInterfaces): Is this doing the right thing for 'allows ref struct' type parameters?
if (refKind != RefKind.None || type.IsRestrictedType())
Copy link
Member

@cston cston Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (refKind != RefKind.None || type.IsRestrictedType())

Are we testing ErrorCode.ERR_AnonDelegateCantUseLocal? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we testing ErrorCode.ERR_AnonDelegateCantUseLocal?

No, I think this code path is covered by a test for ERR_AnonDelegateCantUseRefLike.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add the following test in the next PR:

        [Fact]
        public void IllegalCapturing_03()
        {
            var source = @"
ref struct R1
{
}

class C
{
    void M<T>(R1 r1, T t)
        where T : allows ref struct
    {
        R1 r2 = r1;
        T t2 = t;

        var d1 = () => r2;
        var d2 = () => t2;
    }
}
";
            var comp = CreateCompilation(source, targetFramework: s_targetFrameworkSupportingByRefLikeGenerics);

            comp.VerifyEmitDiagnostics(
                // (14,24): error CS8175: Cannot use ref local 'r2' inside an anonymous method, lambda expression, or query expression
                //         var d1 = () => r2;
                Diagnostic(ErrorCode.ERR_AnonDelegateCantUseLocal, "r2").WithArguments("r2").WithLocation(14, 24),
                // (15,24): error CS8175: Cannot use ref local 't2' inside an anonymous method, lambda expression, or query expression
                //         var d2 = () => t2;
                Diagnostic(ErrorCode.ERR_AnonDelegateCantUseLocal, "t2").WithArguments("t2").WithLocation(15, 24)
                );
        }

{
}
}
}
Copy link
Member

@cston cston Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider testing:

class Helper1<T, U>
    where T : allows ref struct
    where U : T, allows ref struct
{
    public static void Test1(T h1)
    {
        if (h1 is U)
        {
        }
    }
    public static void Test2(U h2)
    {
        if (h2 is T)
        {
        }
    }
}
``` #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider testing:

I will add the following test in the next PR:

        [Fact]
        public void IsOperator_10()
        {
            var src = @"
class Helper1<T, U>
    where T : allows ref struct
    where U : T, allows ref struct
{
    public static void Test1(T h1)
    {
        if (h1 is U)
        {
        }
    }
    public static void Test2(U h2)
    {
        if (h2 is T)
        {
        }
    }
}
";

            var comp = CreateCompilation(src, targetFramework: s_targetFrameworkSupportingByRefLikeGenerics);
            comp.VerifyDiagnostics(
                // (8,13): error CS0019: Operator 'is' cannot be applied to operands of type 'T' and 'U'
                //         if (h1 is U)
                Diagnostic(ErrorCode.ERR_BadBinaryOps, "h1 is U").WithArguments("is", "T", "U").WithLocation(8, 13),
                // (14,13): error CS0019: Operator 'is' cannot be applied to operands of type 'U' and 'T'
                //         if (h2 is T)
                Diagnostic(ErrorCode.ERR_BadBinaryOps, "h2 is T").WithArguments("is", "U", "T").WithLocation(14, 13)
                );
        }

{
}
}
}
Copy link
Member

@cston cston Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider testing:

class Helper1<T, U>
    where T : allows ref struct
    where U : T, allows ref struct
{
    public static void Test1(T h1)
    {
        if (h1 is U u)
        {
        }
    }
    public static void Test2(U h2)
    {
        if (h2 is T t)
        {
        }
    }
}
``` #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider testing:

I will add the following test in the next PR:

        [Fact]
        public void IsPattern_10()
        {
            var src = @"
class Helper1<T, U>
    where T : allows ref struct
    where U : T, allows ref struct
{
    public static void Test1(T h1)
    {
        if (h1 is U u)
        {
        }
    }
    public static void Test2(U h2)
    {
        if (h2 is T t)
        {
        }
    }
}
";

            var comp = CreateCompilation(src, targetFramework: s_targetFrameworkSupportingByRefLikeGenerics);
            comp.VerifyDiagnostics(
                // (8,19): error CS8121: An expression of type 'T' cannot be handled by a pattern of type 'U'.
                //         if (h1 is U u)
                Diagnostic(ErrorCode.ERR_PatternWrongType, "U").WithArguments("T", "U").WithLocation(8, 19),
                // (14,19): error CS8121: An expression of type 'U' cannot be handled by a pattern of type 'T'.
                //         if (h2 is T t)
                Diagnostic(ErrorCode.ERR_PatternWrongType, "T").WithArguments("U", "T").WithLocation(14, 19)
                );
        }

// if (t is Span<int>)
Diagnostic(ErrorCode.ERR_BadBinaryOps, "t is Span<int>").WithArguments("is", "T", "System.Span<int>").WithLocation(7, 13),
Diagnostic(ErrorCode.WRN_IsAlwaysFalse, "t is Span<int>").WithArguments("System.Span<int>").WithLocation(7, 13),
Copy link
Member

@cston cston Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WRN_IsAlwaysFalse

The severity of the diagnostic for t is Span<int> was reduced from an error to a warning. Will that affect success cases in C# 12 (say overload resolution with lambda arguments)? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WRN_IsAlwaysFalse

The severity of the diagnostic for t is Span<int> was reduced from an error to a warning. Will that affect success cases in C# 12 (say overload resolution with lambda arguments)?

Quite possibly, but we, usually, do not worry about changes in behavior for scenarios like that.

@AlekseyTs AlekseyTs requested a review from cston April 29, 2024 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers New Feature - RefStructInterfaces untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants