Remaining issues with source generation migration

[peppy]

Following up from #3085, I'm hitting a couple more issues.

On peppy/osu@87e2285:

  Fody/Realm: An unexpected error occurred while weaving 'BeatmapCollection': Specified argument was out of the range of valid values..
Callstack:
   at Mono.Collections.Generic.Collection`1.RemoveAt(Int32 index) in C:\projects\fody\cecil\Mono.Collections.Generic\Collection.cs:line 159
   at RealmWeaver.Weaver.RemoveBackingFields(TypeDefinition type, HashSet`1 backingFields) in D:\a\realm-dotnet\realm-dotnet\Realm\Realm.Weaver\RealmWeaver.cs:line 287
   at RealmWeaver.Weaver.WeaveGeneratedType(TypeDefinition type) in D:\a\realm-dotnet\realm-dotnet\Realm\Realm.Weaver\RealmWeaver.cs:line 331
   at RealmWeaver.Weaver.<>c__DisplayClass39_0.<Execute>b__2(MatchingType matchingType) in D:\a\realm-dotnet\realm-dotnet\Realm\Realm.Weaver\RealmWeaver.cs:line 225

  Fody: An unhandled exception occurred:
Exception:
Member 'System.String <Name>k__BackingField' is declared in another module and needs to be imported
Type:
System.ArgumentException
StackTrace:
   at Mono.Cecil.MetadataBuilder.LookupToken(IMetadataTokenProvider provider) in C:\projects\fody\cecil\Mono.Cecil\AssemblyWriter.cs:line 2264
   at Mono.Cecil.Cil.CodeWriter.WriteOperand(Instruction instruction) in C:\projects\fody\cecil\Mono.Cecil.Cil\CodeWriter.cs:line 270
   at Mono.Cecil.Cil.CodeWriter.WriteInstructions() in C:\projects\fody\cecil\Mono.Cecil.Cil\CodeWriter.cs:line 180
   at Mono.Cecil.Cil.CodeWriter.WriteResolvedMethodBody(MethodDefinition method) in C:\projects\fody\cecil\Mono.Cecil.Cil\CodeWriter.cs:line 111
   at Mono.Cecil.Cil.CodeWriter.WriteMethodBody(MethodDefinition method) in C:\projects\fody\cecil\Mono.Cecil.Cil\CodeWriter.cs:line 54
   at Mono.Cecil.MetadataBuilder.AddMethod(MethodDefinition method) in C:\projects\fody\cecil\Mono.Cecil\AssemblyWriter.cs:line 1675
   at Mono.Cecil.MetadataBuilder.AddMethods(TypeDefinition type) in C:\projects\fody\cecil\Mono.Cecil\AssemblyWriter.cs:line 1668
   at Mono.Cecil.MetadataBuilder.AddType(TypeDefinition type) in C:\projects\fody\cecil\Mono.Cecil\AssemblyWriter.cs:line 1468
   at Mono.Cecil.MetadataBuilder.AddTypes() in C:\projects\fody\cecil\Mono.Cecil\AssemblyWriter.cs:line 1440
   at Mono.Cecil.MetadataBuilder.BuildTypes() in C:\projects\fody\cecil\Mono.Cecil\AssemblyWriter.cs:line 1289
   at Mono.Cecil.MetadataBuilder.BuildModule() in C:\projects\fody\cecil\Mono.Cecil\AssemblyWriter.cs:line 1059
   at Mono.Cecil.MetadataBuilder.BuildMetadata() in C:\projects\fody\cecil\Mono.Cecil\AssemblyWriter.cs:line 1029
   at Mono.Cecil.ModuleWriter.<>c.<BuildMetadata>b__2_0(MetadataBuilder builder, MetadataReader _) in C:\projects\fody\cecil\Mono.Cecil\AssemblyWriter.cs:line 147
   at Mono.Cecil.ModuleDefinition.Read[TItem,TRet](TItem item, Func`3 read) in C:\projects\fody\cecil\Mono.Cecil\ModuleDefinition.cs:line 971
   at Mono.Cecil.ModuleWriter.BuildMetadata(ModuleDefinition module, MetadataBuilder metadata) in C:\projects\fody\cecil\Mono.Cecil\AssemblyWriter.cs:line 146
   at Mono.Cecil.ModuleWriter.Write(ModuleDefinition module, Disposable`1 stream, WriterParameters parameters) in C:\projects\fody\cecil\Mono.Cecil\AssemblyWriter.cs:line 119
   at Mono.Cecil.ModuleWriter.WriteModule(ModuleDefinition module, Disposable`1 stream, WriterParameters parameters) in C:\projects\fody\cecil\Mono.Cecil\AssemblyWriter.cs:line 78
   at Mono.Cecil.ModuleDefinition.Write(String fileName, WriterParameters parameters) in C:\projects\fody\cecil\Mono.Cecil\ModuleDefinition.cs:line 1160
   at InnerWeaver.WriteModule() in C:\projects\fody\FodyIsolated\ModuleWriter.cs:line 18
   at InnerWeaver.Execute() in C:\projects\fody\FodyIsolated\InnerWeaver.cs:line 115
Source:
Mono.Cecil
TargetSite:
Mono.Cecil.MetadataToken LookupToken(Mono.Cecil.IMetadataTokenProvider)

I don't think fody should be involved anymore, so I attempted to disable it, which results in a successful compilation, but a new runtime issue:

Unhandled exception. System.ArgumentException: The class ScoreInfo is not in the limited set of classes for this realm (Parameter 'T')
   at Realms.Helpers.Argument.Ensure(Boolean condition, String message, String paramName)
   at Realms.Realm.All[T]()
   at osu.Game.Database.RealmAccess.cleanupPendingDeletions() in /Users/dean/Projects/osu/osu.Game/Database/RealmAccess.cs:line 319
   at osu.Game.Database.RealmAccess..ctor(Storage storage, String filename, GameThread updateThread) in /Users/dean/Projects/osu/osu.Game/Database/RealmAccess.cs:line 216
   at osu.Game.OsuGameBase.load(ReadableKeyCombinationProvider keyCombinationProvider) in /Users/dean/Projects/osu/osu.Game/OsuGameBase.cs:line 258
   at osu.Game.OsuGameBase.<>c.<RegisterForDependencyActivation>b__147_0(Object t, IReadOnlyDependencyContainer d) in /Users/dean/Projects/osu/osu.Game/osu.Framework.SourceGeneration/osu.Framework.SourceGeneration.DependencyInjectionSourceGenerator/g_osu.Game.OsuGameBase_Dependencies.cs:line 16

Here's ScoreInfo.cs on that branch for reference.

Any pointers would be appreciated!

[nirinchev]

Fody is still involved with source generated classes because SG doesn't support the replace keyword, which prevents us from supplying the implementation of the properties defined in your model. I'll take a look at this and report back - as far as I can tell, the issue is with the logic that optimizes the class by removing some backing fields, but it's not obvious to me what's wrong on our end.

[nirinchev]

So the issue seems to be with this line: https://github.com/ppy/osu/blob/d679703fa24dcf8ed12f95febd0eeb962e3552a6/osu.Game/Collections/BeatmapCollection.cs#LL47.

The problem here comes from the interaction of two things we didn't account well for:

1. For source generated classes, we added an optimization that removes the backing fields from the models. The reason is those are never used and with large classes it does show up in benchmarks for GC. It also can result in incorrect behavior because we copy the property initializers into the generated ManagedAccessor. This means that the initializer would run twice for every instance created.
2. In the ctor you're setting readonly properties. Since those properties don't have a setter, the compiler will set the backing field directly. This is not something we've accounted for and the weaver logic is incorrect there. Even if we did account for it, things would still end up wrong, because we would discard the collection you're trying to assign the list to.

So this is a very subtle bug that we need to fix - ideally the source generator should detect when you're trying to assign readonly persisted properties and error out with a nice explanation. In the meantime though, the fix for you would be to make sure you don't do that. My suggestion would be to add an extension like:

public static void AddRange<T>(this ICollection<T> collection, IEnumerable<T>? items)
{
    if (items is null)
        return;
    }

    foreach (T obj in items)
        collection.Add(obj);
}

and then use that rather than assignment:

public BeatmapCollection(string? name = null, IList<string>? beatmapMD5Hashes = null)
{
    ID = Guid.NewGuid();
    Name = name ?? string.Empty;
    BeatmapMD5Hashes.AddRange(beatmapMD5Hashes);

    LastModified = DateTimeOffset.UtcNow;
}

I haven't tried to compile all projects in the osu solution, but with that change, osu.Game compiles successfully (though I didn't run it to see if something else is broken).

[peppy]

Missed this reply sorry!

Thanks a lot, this seems to work perfectly.