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

API Change Request: Expression support for ref and readonly ref types #24884

Open
JonHanna opened this issue Feb 2, 2018 · 13 comments
Open

API Change Request: Expression support for ref and readonly ref types #24884

JonHanna opened this issue Feb 2, 2018 · 13 comments
Assignees
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Linq.Expressions blocked Issue/PR is blocked on something - see comments
Milestone

Comments

@JonHanna
Copy link
Contributor

JonHanna commented Feb 2, 2018

Currently expressions represents ref types only in one place, viz. ParameterExpression objects for which IsByRef is true.

With a greater use of ref types due to greater language support, it seems fruitful to increase the availability of ref types in other expressions. While allowing such types directly on expressions' Type would be one reasonable approach (so that e.g. exp.Type == typeof(int).MakeByRefType() could be true) this has two problems:

  1. It doesn't play well with generics, and while generics aren't used in the typing of most expressions, they are important for the type of lambdas, so keeping ref-ness separate is probably cleaner.
  2. (More serious of the two, IMO). Doing this would not blend well with the current implementation of ParameterExpression.

There is also metadata support for ref types for which the address should not be written through (in parameters and ref readonly variables and returns in C#). It would be useful to be able to express this in expressions.

Note that #24621 would require such ref support as a prerequisite to making it available to Microsoft.CSharp.

This proposal suggests adding a IsByRef property to Expression similar to that of ParameterExpression (which would hide it in that case, but should return the save value) to represent ref types and an IsByRefReadOnly property to represent in/ref readonly types.

The default implementation is given as part of the proposal as that would affect custom classes derived from Expression.

IsByRefReadOnly only varies in the case where IsByRef is true. If IsByRef is false then IsByRefReadOnly should always be false.

GetDelegateType would need at least an internal overload that indicated the readonly quality of in parameters and ref readonly returns, so it should probably be made public.

The Parameter factory would similarly need to be able to indicate in parameters.

Other factories will depend on inferring such types, unless experience shows that explicit factories are beneficial.

namespace System.Linq.Expressions
{
    public partial class Expression
    {
        public static Type GetDelegateType(Type[] typeArgs, bool[] isReadOnlyRef);
        public virtual bool IsByRef { get; }
        public virtual bool IsByRefReadOnly { get; }
        public static ParameterExpression Parameter(Type type, bool isReadOnlyRef);
        public static ParameterExpression Parameter(Type type, bool isReadOnlyRef, string name);
    }

    public class ParameterExpression : Expression
    {
        public new bool IsByRef { get; } // new added to prevent CS0108
    }
}
@OmarTawfik
Copy link
Contributor

I think we should change it to IsByRefReadOnly (same in Roslyn APIs) instead of IsReadOnlyType, which might get confusing, specially with the addition of readonly structs. In that case, IsByRefReadOnly would also be mutually exclusive with IsByRef.

@JonHanna
Copy link
Contributor Author

Good idea @OmarTawfik and I can't say I was overly happy with IsReadOnlyType when I suggested it. Proposal updated.

@OmarTawfik
Copy link
Contributor

Can you please update the rest of the proposal if you agree with my previous comment? for example:
IsByRefReadOnly only varies in the case where IsByRef is true. If IsByRef is false then IsReadonlyType should always be false.

@JonHanna
Copy link
Contributor Author

Oops. I think that's the only one I missed (fixed now) unless there's "Paris in the the Spring" mistakes left.

@OmarTawfik
Copy link
Contributor

cc @VSadov in case he had comments on the API.

@terrajobst
Copy link
Member

@jaredpar, based on our conversation it seems the language team + consumers (e.g. EF) should drive the design. Is that you or someone in particular we should tag?

@VSadov
Copy link
Member

VSadov commented Sep 26, 2018

Attempts to support ref in S.L.E quickly runs into issues with poor support for ref in Reflection.
See https://github.com/dotnet/corefx/issues/4411 and linked bugs/discussions.

All that reflection has in this area is copyback via an object array for ref parameters. That is slow and breaks aliasing, which is often the main point of ref, - try using Interlocked.CompareExchange through reflection to see what I mean. For ref returns the aliasing-preserving API is basically a requirement.

Are we starting on the refs-in-reflection issue?

@VSadov
Copy link
Member

VSadov commented Sep 26, 2018

Ignoring the reflection issues, the proposal here makes sense. It may be worth some prototyping to see if there is something missing.

I think there will be a need for an assignment expression to be able to tell whether it is a ref or ordinary byval assignment as otherwise we may see ambiguous situations.

@terrajobst
Copy link
Member

terrajobst commented Aug 27, 2019

Video

There is a general item on the compiler team to figure out how expression trees are going to be evolved (and if).

@raffaeler
Copy link

@terrajobst you are scaring me and my customers. I built huge pieces of infrastructure for many customers that heavily depends on the lightweight generation and compiling offered by the expression trees.
In several discussions with @MadsTorgersen I asked multiple times to give the expression trees the language parity they deserve for the scenario I faced over the last years.
From what I see, the expression trees are a huge plus for the .NET platform adoption.
Of course, whenever I can, I generate and compile code with Roslyn, but the difference in terms of performance and memory is huge and too often I can't use it (sometimes I mixed the two).
I hope the team will consider not only this thread proposal, but the full language parity (await support is another big one for example)

@terrajobst
Copy link
Member

terrajobst commented Aug 28, 2019

The reason we haven't touched expression trees in a while is precisely because out of fear of breaking consumers. We have seen cases where Roslyn's code gen for the trees was slightly different from the original C# compiler that was implemented in C++ and that broke consumers because they made assumptions around the shape of those trees.

It seems to me that updating this feature makes sense, but it's a work item that is larger than us just changing the APIs. We have to coordinate that change with the compiler teams and with popular linq providers -- otherwise it's not helping anyone.

@raffaeler
Copy link

I totally understand your points. While retro-compatibility on the current implementation is very important, I would welcome any of the two possible solutions coming in my mind:

  • create a new API with similar characteristics but giving language parity:

    • typed during construction is important because it gives stronger guarantees about the correctness and the culcprit of the error. This is the opposite of what happens in Roslyn where I can build a bad AST in memory that will not compile and that ius much harder to fix when the code is automatically generated.
    • very lightweight and very fast compilation (which is the reason why most people is using Expressions today)
    • filling the gaps with the .NET Framework implementation (debug support in primis)
    • ability to modify/transform/transpile the in-memory representation of the AST
  • reusing the current API but adding the new C# features in a different flavors, given the problems you mentioned.

Probably choosing to create a new API would be not welcome from many because it would require migrating (very complex) code. But I would personally be happy to migrate in change of powerful features like:

  • in the box transpiling to and from Roslyn AST
  • built-in support from C# language to give a better developer experience in terms of metaprogramming
  • better performance

Thanks

@bartdesmet
Copy link
Contributor

Linking dotnet/csharplang#2545 which covers this from the language side and has further links to prototypes and proposals, including:

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@cston cston modified the milestones: 5.0.0, Future Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Linq.Expressions blocked Issue/PR is blocked on something - see comments
Projects
None yet
Development

No branches or pull requests

8 participants