dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.85k stars 4k forks source link

Immutable C# "Init-Only" Records "should just work" **WHEN CONSUMED** by VB... but the opposite appears to be true. #49469

Closed DualBrain closed 3 years ago

DualBrain commented 3 years ago

I was in the middle of demonstrating how you shouldn’t be afraid to “mix” languages and, thus, take advantage of new time saving features that have been recently introduced. The scenario that came to mind (adhoc) was leveraging “records” by creating a simple class library in C# and create several records that could then be utilized (with all the benefits – at least that was the thought) that records brings with equality, immutability, etc.

The way they are being demo'd during the launch and written about in pretty much every blog post that I'm finding... if these classes are made visible/public in libraries in the manner being demo'd... VisualBasic will suffer big time as I see it. If it's adopted heavily by library makers; we are in serious trouble.

As I currently see it, there's no way to initialize this immutable object (record). If people go gung-ho with utilizing this as I'd expect you would encourage; we will have no way to initialize these things (unless I’m completely missing something). That's a HUGE problem!

Version Used:

.NET 5, VS 16.8.1

Steps to Reproduce:

Create a simple .NET 5 C# class library project with a public record:

public record Something {public int ID { get; init; }}

Create a .NET 5 VB console project, add a reference to the C# project.

Attempt to consume the record:

Dim c = New Something With {.ID = 1}

Expected Behavior:

Ability to initialize the record using the, what I would think would be, valid/expected code

Actual Behavior:

Unable to initialize the record due to the following (rather confusing) error:

'ID' has a return type that is not supported or parameter types that are not supported.

TAG: @KathleenDollard

Dotnet-GitSync-Bot commented 3 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

DualBrain commented 3 years ago

I should add that the record "works fine" if I change the init to set for the "properties". Of course, that then removes the immutability. :-( Furthermore, if I don't have access to the library source to "just change it from init to set"... it effectively blocks VB from consuming anything that this is utilized where the expectation is to have the consumer of the library create/initialize the record. (Which I believe could/would happen often if records become as popular as I would assume.)

CyrusNajmabadi commented 3 years ago

VB and C# are different languages. There is no expectation or guarantee that you would write the same constructs in the same way when moving between them. Instead, you should use whatever forms are idiomatic for that language.

CyrusNajmabadi commented 3 years ago

reactivating. i didn't mean to close this. We'll have a fully fleshed out response on the expected side of things here for VB.

DualBrain commented 3 years ago

VB and C# are different languages. There is no expectation or guarantee that you would write the same constructs in the same way when moving between them. Instead, you should use whatever forms are idiomatic for that language.

I have project A that is written in C#. The expectation is that I'm creating a reusable library that will be consumed by others.

I have project B, that happens to be written in VB. When I attempt to take advantage of the exposed class (a "record") in VB that is initialized with "with"... I'm unable to do so as desired due to the error message received. I'm not trying to write the same thing in VB that I wrote in C#. if I could do that, I'd just write it in VB to begin with.

The problem is that if records are adopted as I'd expect the C# team hopes and library providers build libraries expecting the ability to initialize one of these classes through the property initializers only... VB is SCREWED!!!!!

This is broken. This really should be addressed. It should be possible to initialize these classes from VB... they are "just classes" afterall. Additionally, it should not be expected of a C# developer to a) know that this is a problem, b) change their code to work by having to build (overloaded) constructors and c) being a VB developer automatically making you hamstrung and restricted from using libraries because something wasn't thought through and addressed from the beginning.

This isn't about C# versus VB... this is about the overall .NET ecosystem. Stuff like this shouldn't be broken.

CyrusNajmabadi commented 3 years ago

this is about the overall .NET ecosystem.

In general, every language will need to decide how they want to deal with things here. Including how to deal with constructs that have mod-reqs on them.

As mentioned before though, we will give some more info on this soon.

DualBrain commented 3 years ago

Can the tags for "Resolution-Answered" and "Resolution-By Design" be removed until the "more info on this" occurs?

Thanks.

HaloFour commented 3 years ago

@DualBrain

There are quite a few features available to C# that can't be consumed easily (if at all) from VB.NET, like pointers, ref returns, ref structs, in parameters, etc. Granted, those are much more special-purpose than records, which are more like POCO 2.0.

Some backstory:

https://github.com/dotnet/csharplang/issues/1689 https://github.com/dotnet/csharplang/blob/master/meetings/2020/LDM-2020-03-23.md#builder-based-records https://github.com/dotnet/csharplang/blob/master/meetings/2020/LDM-2020-03-30.md#builders-vs-init-only https://github.com/dotnet/csharplang/issues/3376

VBAndCs commented 3 years ago

I think the whole record design is a mess. It is too limited and too problematic. I created a demo for an alternative design in VB.NET, that gave the same benefits with more flexibility, with no need at all to the init only properties:

and this is the generated code:

Public Class Info
   Public ReadOnly Property X As Integer
   Public ReadOnly Property Y As Object
   Public ReadOnly Property Z As Date

   Public Sub New(x As Integer, Optional y As Object = "Ali", Optional z As Date = Now)
      _X = X
      _Y = Y
      _Z = Z
   End Sub

   Public Shared Function From(anotherRecord As Info, Optional X As Integer? = Nothing, Optional Y As Object? = Nothing, Optional Z As Date? = Nothing) As Info
      Return New Info(      If(X Is Nothing, anotherRecord.X, X),       If(Y Is Nothing, anotherRecord.Y, Y),       If(Z Is Nothing, anotherRecord.Z, Z))
   End Function

    Public Overrides Function Equals(anotherObject) As Boolean
            Dim anotherRecord = TryCast(anotherObject, Info)
            If anotherRecord Is Nothing Then Return False
            Return Equals(anotherRecord)
        End Function

   Public Overloads Function Equals(anotherRecord As {R.Name}) As Boolean
      If Not X.Equals(anotherRecord.X) Then Return False
      If Not Y.Equals(anotherRecord.Y) Then Return False
      If Not Z.Equals(anotherRecord.Z) Then Return False
      Return True
   End Function

   Public Shared Widening Operator CType(anotherRecord As Info) As (X As Integer, Y As Object)
      Return (anotherRecord.X, anotherRecord.Y)
   End Operator

   Public Shared Widening Operator CType(fromTuple As (X As Integer, Y As Object)) As Info
      Return new Info(fromTuple.X, fromTuple.Y)
   End Operator

   Public Shared Function From(fromTuple As (X As Integer, Y As Object)) As Info
      Return new Info(fromTuple.X, fromTuple.Y)
   End Function

   Public Shared Widening Operator CType(anotherRecord As Info) As (X As Integer, Y As Object, Z As Date)
      Return (anotherRecord.X, anotherRecord.Y, anotherRecord.Z)
   End Operator

   Public Shared Widening Operator CType(fromTuple As (X As Integer, Y As Object, Z As Date)) As Info
      Return new Info(fromTuple.X, fromTuple.Y, fromTuple.Z)
   End Operator

   Public Shared Function From(fromTuple As (X As Integer, Y As Object, Z As Date)) As Info
      Return new Info(fromTuple.X, fromTuple.Y, fromTuple.Z)
   End Function

End  Class

And these are some possible variations of syntax:

' immutable but with no keys, will not generate equals methods, so, it is a regular immutable class (or struct if you use Structure) :
Public Readonly class Info(
     X as Integer, 
     Y = "Ali", 
     Z as Date = Now
)
' Using ReadOnly and Key with members:
Public class Info(
     Key X as Integer, 
     ReadOnly Y = "Ali", 
     ReadOnly key Z as Date = Now
)

Note that I generate one constructor with required params for readonly properties that have no default values, and optional params for other properties. Note also that the From method can't set ref types to nothing unless they are nothing in the cloned record.

So, My question is: why C# complicated it so much, and invented too many new unnecessary concepts, to so such a simple thing? I am afraid that C# took a wrong turn since C# 8.0, and can never recover from that! It is being more gibberish, more complicated, more incomparable with other .NET languages, and way too hard for beginners. Killing VB in such circumstances can make many developers desert .NET. If you are happy of complicating C#, at least let VB.NET as an attractive door for beginners. But note that no one begins with a language without a future, and decays in the market and hiring in companies. On the other hand, if you are shutting down VB, you must bring more of its spirit to C# to attract its developers and beginners. This is obviously not happening.

SimonTouchtech commented 3 years ago

To be clear, this issue is with init-only properties and not with records, right? You could write a struct or a class with an init-only property and you'd get the same error when trying to set it from VB.

VBAndCs commented 3 years ago

@SimonTouchtech I already made it cleat that there is no need for init-only properties. There are several ways to do the job without it:

SimonTouchtech commented 3 years ago

@VBAndCs I didn't respond to you. Whether you see a need for init-only properties or not they are a thing that exist in C#.. As @DualBrain shows they don't work well when referenced from VB. From some parts of the previous discussion it sounds like the issue is with records, but from what I can tell the issue is actually with init-only properties and I just wanted to clarify whether that's the case or if I misunderstood something about the issue.

fubar-coder commented 3 years ago

I'd suggest to change the title, because it's about init-only properties and not about records (which work just fine). And I also agree with @DualBrain that init-only properties should be initializable by VB, because interop was the selling point of the .NET Framework.

EDIT: @VBAndCs: Your last translation example is wrong. It should be Dim x = MyRec.From(new MyRec(), A:=1, B:=2), because Nothing would cause a NullReferenceException for every property which is not specified in the From function.

HaloFour commented 3 years ago

@VBAndCs

Having a static From method for cloning the record completely breaks in cases of inheritance and causes decapitation. If you had a Person variable containing a Student record the compiler would call Person.From because it wouldn't know that it was a Student which would cause the result to be a Person, not a Student, and none of the Student properties would be copied.

Those kinds of approaches (and many others) were already discussed by the C# team at length. Even if you could solve for all of the identified problems with that approach it would still only result in yet another incompatible implementation that then C# would not understand. Given init properties are here to stay and need changes to interop with C# anyway it'd be easier to update VB.NET to understand the new modreq so that VB.NET can correctly interop with them.

VBAndCs commented 3 years ago

it'd be easier to update VB.NET to understand the new modreq so that VB.NET can correctly interop with them.

That would be nice but:

Going forward, we do not plan to evolve Visual Basic as a language. This supports language stability and maintains compatibility between the .NET Core and .NET Framework versions of Visual Basic. Future features of .NET Core that require language changes may not be supported in Visual Basic. Due to differences in the platform, there will be some differences between Visual Basic on .NET Framework and .NET Core.

We don't know why this policy is enforced in first place, but it is obvious it will cause a lot of damage sooner that planned. VB.NET will need a 20 years of decaying until companies can retire there active projects, and while that a lot of issues will appear because VB can't consume other new libraries. This will force VB developers directly to other VB implementations like Mercury. C# is only an option for new projects but not for existing one. Otherwise, MS will have to keep changing VB to consume new features (without being able to create them) which is a lot of effort and money with less benefit! I think MS has to recalculate its strategy before its too late.

HaloFour commented 3 years ago

@VBAndCs

I agree that the statement is pretty bleak. Members of the team have expressed that VB.NET would still get changes that would allow it to interop with the ecosystem and new features released by C#, but the proof of that will be in how they handle this situation. VB.NET is very unlikely to evolve new features of its own, and I highly doubt that a new implementation of records would be considered for that language. Supporting the consumption of records from C# would be complicated enough (what with appropriate support for the modreq), even if VB.NET can't declare them itself. MS knew that all of their flagship languages would require updates and frankly I'm immensely disappointed that this didn't happen before C# 9.0 shipped.

PathogenDavid commented 3 years ago

(For anyone else who hadn't read that VB feature freeze statement before, it comes from this blog post, GitHub discussion here: https://github.com/dotnet/vblang/issues/497)

DualBrain commented 3 years ago

I'd suggest to change the title, because it's about init-only properties and not about records (which work just fine). And I also agree with @DualBrain that init-only properties should be initializable by VB, because interop was the selling point of the .NET Framework.

Done.

DualBrain commented 3 years ago

To be clear, I'm not looking to get records into VB. This is about interop. This is about co-existing in the .NET ecosystem. "Init-only" (immutable) records is a feature that, if it "blows up" in adoption, can significantly restrict what VB has access to. Immutability is something that is continuing to gain momentum (over the past several years). This style of initializing classes instances has also been gaining momentum in the past several years. I am having a hard time seeing how this situation isn't going to be a major problem going forward. Thus we need changes to take place in VB to support this scenario. This is an interop problem... not a "language" problem. I'm not asking for ANYTHING to change with the "language" - no new "language" feature required. As I said in the original title... it "should just work". We already have several scenarios in VB where this is the case (specifically with interop scenarios). To VB, these should be as they are already described... they are simply classes. They just require initializing a certain way and we already have the "language" structure to support this... the interop story just needs to be fixed. I get that this is something that isn't that simple... but that doesn't change the fact that this is a problem and it is something that should get addressed.

CyrusNajmabadi commented 3 years ago

This is an interop problem... not a "language" problem. I'm not asking for ANYTHING to change with the "language" - no new "language" feature required. As I said in the original title... it "should just work".

Understood. As I mentioned earlier, more information will be forthcoming. You're asking at a time when people are working on a lot of things, and Thanksgiving is right around the corner. So it may not be the case that your get a response 10 minutes later :-)

DualBrain commented 3 years ago

@CyrusNajmabadi Understood. Mainly responding/conversing with others.

VBAndCs commented 3 years ago

In fact, the whole init-only concept seems useless to me. If I want to use a mutable object as if it were immutable or vise versa, it is totally my responsibility. If I am using multi-threading, I will take all the precautions to be safe. Init-only is a fake illusions in .NET and can easily bypassed using reflection. So, If you want to change the value of such properties, just use this function:

    Sub SetPropertyValue(Obj As Object, PropertyName As String, Value As Object)
        Dim tip As Type = Obj.GetType()
        Dim pr As PropertyInfo = tip.GetProperty(PropertyName, BindingFlags.Public Or BindingFlags.Instance Or BindingFlags.NonPublic)
        pr.SetValue(Obj, Value, Nothing)
    End Sub

I tried it with init-only property and it worked. So, Why to bother? What exactly this init-only prevent?

VBAndCs commented 3 years ago

ReadOnly properties in C# don't have setters, but they can be set somehow in the constructor (via the back filed, I think). The init properties doesn't follow the same pattern, so, the property is in fact writable (has a setter!!). This needs a change in CLR, to add a new Init_Prop method instead of the SetProp method, which needs some changes in reflection and languages! But I think it doesn't worth the trouble in first place. The compiler can produce a constructor to initialize all properties using the backfields if the class is written in the project , or use reflection to set the backfield if it can know the backfields and there is no constructor that can do the job. In VB this is easy as it has the same name with a leading `` . So, the initialization code is lowered to a call to a constructor, or a From method (like my demo).

VBAndCs commented 3 years ago

Another trick without reflection (but using late binding):

Dim c  As object = New Something 
c.Set_ID(1)

Works like a charm!

In fact. I with that VB and C# allow access to the Set_Property methods (in early binding). I needed this recently (but can't remember the exact case), and it can be used to define attached property setters and getters.

VBAndCs commented 3 years ago

Another surprise: this also works via late binding:

Dim c  As object = New Something 
c.ID = 1

!!!

Note: I tried the same late binding in C# using dynamic but I got a runtime error.

VBAndCs commented 3 years ago

So, the current workaround is to define an Init attached method to initialize init-only properties, to make late binding less dangerous: Option Strict Off Imports System.Runtime.CompilerServices

Module InitOnly
    <Extension>
    Public Sub Init(x As Something, id As Integer, foo as string)
        Dim O As Object = x
        O.ID = id
        O.Foo = foo
    End Sub

End Module

There should be an overload for each external type containing init-only properties, which can be a tedious work over time, that needs an auto generator, or a refactoring action in VS.NET. Now, we can write:

Dim c as new Something
C.Init(1, "test")

It is of course possible to call the Init method at any time!

HaloFour commented 3 years ago

@VBAndCs

The design of init properties has already shipped and won't be changed. In fact most of what you're finding had already been identified and discussed by the team: https://github.com/dotnet/csharplang/issues/3376. The ability for the setter to be called directly is intentional; the team felt that there was a benefit for init properties to "just work" with existing serialization libraries that rely on reflection. That does mean that there are opportunities for backdoors to mutating those properties, such as VB.NET late-binding, and unless F# was updated it also didn't respect the modreq and would allow writing to the property. The C# dynamic binder was caught before shipping, but that too didn't respect the modreq. I do think that it's unfortunate that the team is relying on these obscure signature enforcement tools, especially since it's apparent that the ecosystem often doesn't, including the Microsoft flagship compilers, but that's the direction they've decided to take.

VBAndCs commented 3 years ago

@HaloFour Inner design can change any time without causing a big damage. But my bigger concern here is not to repeat these mistakes in VB.NET (or any other possible VB implementations). This is why I am making some effort to imagine a suitable alternative design that doesn't pollute the language with unnecessary complicated syntax and concepts.

HaloFour commented 3 years ago

@VBAndCs

Inner design can change any time without causing a big damage.

The feature has shipped and the current design is publicly observable (and has to be). The design cannot be changed without breaking it. Given that these concerns were explicitly considered during the design phase I seriously doubt that there's anything here that would cause the C# team to change their mind. Certainly not to the extent of breaking any project that may already be using records in C# 9.0.

But my bigger concern here is not to repeat these mistakes in VB.NET (or any other possible VB implementations).

VB.NET is not going to get its own implementation of records so there is no question of design there. At best VB.NET will be modified to some degree to understand C# records better, but it's possible that this won't happen either. VB.NET can already consume records and init properties and that might be considered enough.

VBAndCs commented 3 years ago

VB.NET is not going to get its own implementation of records

Not by Microsoft, sure. But there are new alternatives and I expect more. And I expect that VB.NET community will fork the language and evolve it. VB.NET is bigger than death :)

HaloFour commented 3 years ago

@VBAndCs

Not by Microsoft, sure. But there are new alternatives and I expect more. And I expect that VB.NET community will fork the language and evolve it. VB.NET is bigger than death :)

If someone wants to fork the Roslyn compiler and make their own language based on VB.NET that's their prerogative. They can feel free to implement anything that they want. But it'll never be the VB.NET that ships with Visual Studio so it's highly unlikely that it'll gain much traction outside of very niche enthusiast projects. And that conversation isn't particularly relevant to Microsoft's position as to how records will (or won't) be supported in the official language.

DualBrain commented 3 years ago

@CyrusNajmabadi @KathleenDollard

According to this page: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-9.0/init

It states in the Modreqs vs attributes section...

"The primary languages for .NET (C#, F# and VB) will all be updated to recognize these init accessors. Hence the only realistic scenario here is when a C# 9 compiler emits init properties and they are seen by an older toolset such as C# 8, VB 15, etc ... C# 8. That is the trade off to consider and weigh against binary compatibility."

So it looks like, at least at some point, this was on someone's radar.

DualBrain commented 3 years ago

Understood. As I mentioned earlier, more information will be forthcoming. You're asking at a time when people are working on a lot of things, and Thanksgiving is right around the corner. So it may not be the case that your get a response 10 minutes later :-)

Just following up to see if anything has transpired regarding this (other than it being moved to the "BackLog")...

Thanks.

DualBrain commented 3 years ago

Many thanks to the team for getting this working as of 16.9.4!

paul1956 commented 3 years ago

Thanks for the fix.