Provide a WholeProjectDecompiler constructor with no default parameter values

[ds5678]

Problem

I'm trying to trim types from my application, and I'm having issues with the NativeAOT compiler not realizing that my parameters are never null.

ILSpy/ICSharpCode.Decompiler/CSharp/ProjectDecompiler/WholeProjectDecompiler.cs

Line 130 in 9898add

AssemblyReferenceClassifier = assemblyReferenceClassifier ?? new AssemblyReferenceClassifier();

ILSpy/ICSharpCode.Decompiler/CSharp/ProjectDecompiler/WholeProjectDecompiler.cs

Line 132 in 9898add

this.projectWriter = projectWriter ?? (Settings.UseSdkStyleProjectFormat ? ProjectFileWriterSdkStyle.Create() : ProjectFileWriterDefault.Create());

Solution

• I created an interface IAssemblyReferenceClassifier and replaced most uses of AssemblyReferenceClassifier with it.
• I moved default initialization from the most specific WholeProjectDecompiler constructor to the second most one.
• I added ArgumentNullException checks to the most specific WholeProjectDecompiler constructor.

[siegfriedpammer]

isn't this a compile-time silent breaking change, since now anyone deriving from WholeProjectDecompiler using the protected ctor, will get exceptions in their face, if they relied on the default initialization of AssemblyReferenceClassifier and projectWriter?

[ds5678]

I'm not really sure how nullable annotations work for the repository (because of the reference assembly annotation package), but ideally it would go from maybe-null to not-null, so any callers who provide a maybe-null argument will get a warning at compile time.

[siegfriedpammer]

the reference annotator package just copies all the annotations from a given reference framework to the target framework of the decompiler (netstandard2.0) at build-time so Roslyn sees the annotated variants when compiling the decompiler project.

The only issue is that we haven't really invested in annotating everything and there's multiple reasons:

• It's a ton of work
• The old AST model used a null object pattern, making NRT somewhat useless
• Most transforms use a dataflow pattern, that's not really NRT friendly: fields are set in Run and are effectively non-null for the entire duration of the transform (and therefore safe), but this cannot be expressed nicely just by sprinkling ? and some extra attributes.

Given that the new AST model actively uses NRT, it might be worth asking Claude to try to annotate everything.

But as it stands now this is a somewhat ugly breaking change, also third-party users have no way of instantiating the project writer and classifier defaults, afaik...

[ds5678]

But as it stands now this is a somewhat ugly breaking change

I tried to make it as minimally breaking as possible, but I suspected that this might be the answer.

third-party users have no way of instantiating the project writer and classifier defaults, afaik...

The classifier is public, but yes the project writers are internal. Is it intended for them to be an implementation detail?

[siegfriedpammer]

I think we could make the project writer API public too... git history of the whole feature shows that all the other parts have been made public already. So... are we fine with just public or do we unseal at the same time?

[ds5678]

are we fine with just public or do we unseal at the same time?

I'm leaning towards not unsealing because that involves decisions about what the API should be. However, one of my implementations involved a fair bit of copy-pasting. Do you think it would benefit from inheritance?

https://github.com/AssetRipper/AssetRipper/blob/0730de9853007bf255363cb68eadb05e11cf10a3/Source/AssetRipper.Export.PrimaryContent/Scripts/ProjectFileWriter.cs

I have another implementation for IProjectFileWriter. Do you think it should be included upstream?

https://github.com/AssetRipper/AssetRipper/blob/0730de9853007bf255363cb68eadb05e11cf10a3/Source/AssetRipper.Export/Scripts/NullProjectFileWriter.cs

[siegfriedpammer]

WriteFrameworkInfo sounds like a strong candidate for an extensibility point...

Ideas:

• virtual void WriteCustomPropertyGroup(XmlTextWriter xml, ...); which allows adding one more PropertyGroup. And we'd probably need the same a custom ItemGroup?
• maybe model properties and items using an object model that is then serialized to XML

I think the template method approach might be sufficient... might be a bit weird, if we want to avoid <PropertyGroup></PropertyGroup> if the method is not overridden.

Do you have a better idea perhaps?