-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
C# 13: Allows ref struct. #18385
base: main
Are you sure you want to change the base?
C# 13: Allows ref struct. #18385
Conversation
70f2958
to
fd26958
Compare
…meter constraint.
fd26958
to
d336e1d
Compare
d336e1d
to
caaf291
Compare
DCA looks good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 20 changed files in this pull request and generated no comments.
Files not reviewed (15)
- csharp/ql/lib/semmle/code/csharp/Conversion.qll: Language not supported
- csharp/ql/lib/semmle/code/csharp/Generics.qll: Language not supported
- csharp/ql/lib/semmle/code/csharp/Type.qll: Language not supported
- csharp/ql/lib/semmle/code/csharp/Unification.qll: Language not supported
- csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll: Language not supported
- csharp/ql/test/library-tests/csharp11/PrintAst.expected: Language not supported
- csharp/ql/test/library-tests/csharp7.2/PrintAst.expected: Language not supported
- csharp/ql/test/library-tests/csharp7.2/RefStructs.ql: Language not supported
- csharp/ql/test/library-tests/dispatch/CallContext.expected: Language not supported
- csharp/ql/test/library-tests/dispatch/CallGraph.expected: Language not supported
- csharp/ql/test/library-tests/dispatch/GetADynamicTarget.expected: Language not supported
- csharp/ql/test/library-tests/typeparameterconstraints/typeParameterConstraints.expected: Language not supported
- csharp/ql/test/library-tests/typeparameterconstraints/typeParameterConstraints.ql: Language not supported
- csharp/ql/test/library-tests/unification/Unification.expected: Language not supported
- csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/TypeParameterConstraints.cs: Evaluated as low risk
Comments suppressed due to low confidence (4)
csharp/ql/test/library-tests/typeparameterconstraints/TypeParameterConstraints.cs:18
- [nitpick] The term 'allows ref struct' might be confusing. Consider renaming it to something more intuitive.
public void M7<T7>(T7 x) where T7 : allows ref struct { }
csharp/ql/test/library-tests/typeparameterconstraints/TypeParameterConstraints.cs:18
- Ensure that the new 'allows ref struct' constraint is properly tested.
public void M7<T7>(T7 x) where T7 : allows ref struct { }
csharp/ql/test/library-tests/dispatch/ViableCallable.cs:617
- [nitpick] The comment could be clearer by specifying the viable callables more explicitly. Suggest changing to: 'Viable callables: A1.M(), A2.M()'.
// Viable callable: {A1, A2}.M()
csharp/ql/test/library-tests/conversion/boxing/Boxing.cs:50
- The term 'valuetype' should be 'ValueType' to maintain consistency with C# naming conventions.
ref struct S { }
Tip: Turn on automatic Copilot reviews for this repository to get quick feedback on every pull request. Learn more
|
||
override string getAPrimaryQlClass() { result = "RefStruct" } | ||
|
||
override predicate isValueType() { none() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we consider this a value type?
using System;
void M(C c)
{
c.F = 5;
}
var c = new C();
M(c); // Passed by value
Console.WriteLine(c.F); // So prints 0
ref struct C
{
public int F;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely up for debate.
As you write, the ref struct
still exhibits call-by-value semantics. However, we also decided a while back that we needed to introduce post update nodes for ref structs parameters (as ref structs
can have ref
fields - and in this case only the reference is copied, which from a dataflow perspective means that we "can" update a ref struct (or at least update the content of the shared references)). Furthermore, ref structs
can't be boxed (implicitly converted) to ValueType
(or object
). My thinking is that it is "something in between" and "reference like" (this is also the terminology used in Roslyn). So I thought that it was starting to get a bit dangerous to assume that it behaves like a value type both from a type and data flow perspective.
In this PR we introduce support for the
allows ref struct
type parameter constraint. The language feature is described here.A couple of notes on the implementation.
ref struct
type can not be implicitly converted to adynamic
type,object
orValueType
.allows ref struct
is a negative constraint meaning that it extends the number of types that can be used as type replacement for a type parameter.The unification and dispatch call logic has been adapted to take the
allows ref struct
constraint into account (relevant for dynamic dispatch) for deciding relevant dispatch targets. To make things easier a new classRefStruct
has been introduced in the type hierarcy. One could consider whether we want to change the type hierarchy such thatRefStruct
doesn't extendStruct
(and therebyValueType
). Not sure whether this is worth it.Furthermore, we also extract the
notnull
general type parameter constraint.