GenAPI: synthesize private fields for value types #30362
Conversation
There was a problem hiding this comment.
Can you please format your changes:
- (C# coding conventions](https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/coding-style/coding-conventions) -> no lowercase methods, method brackets in a new line, etc.
- Sort the usings at the top
There was a problem hiding this comment.
I added some comments. One thing that this could use in general is more comments around what this is doing and why. The code being ported was pretty extensively commented (https://github.com/dotnet/arcade/blob/aa90e21c63248b4d6d61e8de14a0d8e7a9275130/src/Microsoft.Cci.Extensions/Writers/CSharp/CSharpWriter.cs#L195) because the thing this is doing is very unusual. It's not just sub-setting what's passed in, its constructing replacement API and we should be very clear why it's doing that.
| if (hasRefPrivateField) | ||
| { | ||
| // add reference type dummy field | ||
| yield return dummyField("object", "_dummy", new()); |
There was a problem hiding this comment.
Consider adding constants for these _dummy names. There might be some roslyn API that makes passing in known/special types like int and object easier without having to refer to them as strings and parse those.
There was a problem hiding this comment.
There is, it looks like
SyntaxFactory.PredefinedType(SyntaxFactory.Token(SyntaxKind.IntKeyword))The issue is that it doesn't easily generalize in a way where I can pass the generic type/field names to the dummyField method. I could however add some string constants here for "object", "_dummy", "int", and "_dummyPrimitive".
src/GenAPI/genapi.slnf
Outdated
| "src\\Tests\\Microsoft.NET.TestFramework\\Microsoft.NET.TestFramework.csproj", | ||
| ] | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
That file was actually quite nice. Feel free to bring it back if you want to. We have the same for apicompat.
There was a problem hiding this comment.
Unfortunately, loading this file into VS kept crashing it, so I opted to remove it.
It doesn't crash for apicompat.slnf, so I'd have to figure out if something's missing.
Private fields in a value type (like a struct) are part of its API contract.
Fixes dotnet/arcade#11347