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

Allow ref structs to implement interfaces, respect "allows ref struct" during constraints check. #72537

Merged

Conversation

AlekseyTs
Copy link
Contributor

No description provided.

@AlekseyTs AlekseyTs requested a review from a team as a code owner March 14, 2024 04:41
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Mar 14, 2024
@AlekseyTs
Copy link
Contributor Author

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

@@ -7917,4 +7914,13 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_ClassIsCombinedWithRefStruct" xml:space="preserve">
<value>Cannot allow ref structs for a type parameter known from other constraints to be a class</value>
</data>
<data name="ERR_NotRefStructConstraintNotSatisfied" xml:space="preserve">
<value>The type '{2}' may not be a ref struct or a type parameter allowing ref structs in order to use it as parameter '{1}' in the generic type or method '{0}'</value>
Copy link
Member

@cston cston Mar 14, 2024

Choose a reason for hiding this comment

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

The type '{2}' may not be a ref struct or a type parameter allowing ref structs in order to use it as parameter '{1}' in the generic type or method '{0}'

Perhaps: "The type ... must not be a ref struct ... in order to use it as type parameter ..." #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.

For "may not be a", I think I used the following message as a template:

  <data name="ERR_LambdaInIsAs" xml:space="preserve">
    <value>The first operand of an 'is' or 'as' operator may not be a lambda expression, anonymous method, or method group.</value>
  </data>

For "in order to use it as parameter", there is a "prior art" too:

  <data name="ERR_RefConstraintNotSatisfied" xml:space="preserve">
    <value>The type '{2}' must be a reference type in order to use it as parameter '{1}' in the generic type or method '{0}'</value>
  </data>

I can make the suggested change if you still think it is needed. Please let me know.

// PROTOTYPE(RefStructInterfaces): The boxing should be disallowed.
comp.VerifyDiagnostics(
);
}
Copy link
Member

@cston cston Mar 14, 2024

Choose a reason for hiding this comment

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

}

Consider testing explicit cast as well: return (object)x;. #Resolved

Copy link
Member

Choose a reason for hiding this comment

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

Are we testing boxing for this?

interface I { }
ref struct R : I
{
    I ImplicitCast => this;
    I ExplicitCast => (I)this;
}

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 boxing for this?

Why would this make a difference? Conversion from ref struct to an interface is covered in ImplementAnInterface_02_IllegalBoxing. I do not find anything interesting in the suggested scenario. If I am missing something, please let me know.

// PROTOTYPE(RefStructInterfaces): The boxing should be disallowed.
comp.VerifyDiagnostics(
);
}
Copy link
Member

@cston cston Mar 14, 2024

Choose a reason for hiding this comment

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

}

Consider testing explicit cast as well: return (I1)x;.

Also:

static U Test1<T, U>(T x)
    where T : allows ref struct
    where U : T
{
    return x;
}
static U Test2<T, U>(T x)
    where T : allows ref struct
    where U : T
{
    return (U)x;
}
``` #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Mar 14, 2024

Choose a reason for hiding this comment

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

Consider testing explicit cast as well: return (I1)x;.

Testing boxing is not a goal of this PR. It is known that proper checks are not in place and the added tests are sufficient to make the knowledge obvious

// _ = typeof(C<S1, S1>);
Diagnostic(ErrorCode.ERR_NotRefStructConstraintNotSatisfied, "S1").WithArguments("C<T, S>", "S", "S1").WithLocation(12, 26)
);
}
Copy link
Member

@cston cston Mar 14, 2024

Choose a reason for hiding this comment

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

}

Consider testing:

_ = typeof(C<T, T>);
_ = typeof(C<S, S>);
``` #Resolved

IL_000e: ret
}
");
}
Copy link
Member

@cston cston Mar 14, 2024

Choose a reason for hiding this comment

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

}

Consider testing variance error cases as well:

interface I<T>
{
    void M(T t);
}
interface IOut<out T>
{
    T MOut();
}
ref struct S : I<object>, IOut<object>
{
    public void M(object o) { }
    public object MOut() => null;
}
class Program
{
    static void Main()
    {
        Test1(new S());
        Test2(new S());
    }
    static void Test1<T>(T x) where T : I<string>, allows ref struct
    {
    }
    static void Test2<T>(T x) where T : IOut<string>, allows ref struct
    {
    }
}``` #Resolved

@AlekseyTs
Copy link
Contributor Author

@jjonescz, @dotnet/roslyn-compiler For the second review.

return true;
}

if (typeArgument.Type.IsRefLikeType && conversions.ImplementsVarianceCompatibleInterface(typeArgument.Type, constraintType.Type, ref useSiteInfo))
Copy link
Member

@jjonescz jjonescz Mar 15, 2024

Choose a reason for hiding this comment

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

Why is this check needed specifically for ref struct type arguments - i.e., why they need different handling then normal structs for example? #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.

Why is this check needed specifically for ref struct type arguments - i.e., why they need different handling then normal structs for example?

There are no boxing conversion for ref structs, therefore, the conversions.HasBoxingConversion check above fails.

Assert.Equal("I1", s1.InterfacesNoUseSiteDiagnostics().Single().ToTestDisplayString());
}

CreateCompilation(src, targetFramework: TargetFramework.Net80, parseOptions: TestOptions.RegularNext).VerifyDiagnostics();
Copy link
Member

@jjonescz jjonescz Mar 15, 2024

Choose a reason for hiding this comment

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

Should we also test RegularPreview to ensure this keeps working in C#14+? #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.

Should we also test RegularPreview to ensure this keeps working in C# 14+?

It is tested, that is the default option for our tests. Therefore, the CompileAndVerify covers that

Copy link
Member

Choose a reason for hiding this comment

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

I've overlooked that, thanks.

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