Improve primary-constructor decompilation, collection expressions, and project export - #4049
Improve primary-constructor decompilation, collection expressions, and project export#4049sonyps5201314 wants to merge 9 commits into
Conversation
|
|
||
| namespace ICSharpCode.Decompiler.CSharp.Transforms | ||
| { | ||
| public sealed class IntroduceCollectionExpressions : DepthFirstAstVisitor, IAstTransform |
There was a problem hiding this comment.
collection expressions should be created in ExpressionBuilder not as a separate transform.
There was a problem hiding this comment.
Agreed. I will not carry this separate transform into the split PRs. Collection expressions are paused; I will open a design issue first and propose creating them in ExpressionBuilder.
| return true; | ||
| } | ||
|
|
||
| void ConvertConstructorInitializerCollections(AstNode invocation) |
There was a problem hiding this comment.
As I said in the other comment, if collection expressions are handled in the ExpressionBuilder, then this is no longer necessary.
There was a problem hiding this comment.
Agreed. This constructor-specific collection reconstruction is excluded from the split work. If the feature resumes after the design discussion, collection-expression nodes will be created in ExpressionBuilder.
|
Thank you for your pull request. Unfortunately, I don't think we can merge it as one single PR. Also it would have been a good idea to start an architecture/design session before starting the implementation of a feature as massive and complex as collection expressions. There are a lot of different code patterns that are produced when collection expressions are involved. As a first step, please split the "primary constructor decompilation" feature out into a separate PR. I will review it separately. Also let's have a discussion on how to implement collection expressions in an issue first. Since you are using AI, would love to hear about your approach. What is the prompt you use? What are the inputs? Do you write test cases first? How do you decide whether the design proposal by the LLM uses the correct layer? You see, when I implement a language feature using an LLM I definitely don't let the LLM have the final say on the architecture. I give strict instructions, where the transformation should happen and usually I have to remind the LLM of special cases and edge cases it misses. Using an LLM to implement features and open pull requests does not free you from the responsibility of guiding the LLM. And in order to guide the LLM correctly, you need to actually know the codebase, architecture and the reasons for the design. I have already commented two hints on the architecture/implementation of collection expresssions, but this is just the tip of the iceberg. Please, let's have a long and detailed discussion first. |
|
I think since you have these specific design requirements, it would be better to place these constraints in an
|
| return null; | ||
| } | ||
|
|
||
| bool IsTrivialRecordAutoProperty(PropertyDeclaration declaration, IProperty property, IField field) |
There was a problem hiding this comment.
in what sense are auto properties different in records?
There was a problem hiding this comment.
They are not fundamentally different. This special case tried to ignore outside backing-field references from synthesized record members before those members were removed. That couples two transforms incorrectly, so I am dropping it rather than defending the distinction.
| string StrongNameKeyFile { get; } | ||
| } | ||
|
|
||
| internal interface INullableProjectInfoProvider |
There was a problem hiding this comment.
Agreed; this ad-hoc interface is the wrong abstraction. It existed only to pass the nullable setting without changing the public IProjectInfoProvider contract. I am excluding it from the split PRs and will revisit project export separately through the existing settings/options flow.
| Console.WriteLine(value); | ||
| } | ||
|
|
||
| public void TestUnresolvedStructMemberCalls() |
There was a problem hiding this comment.
this test + fix should be a separate PR
There was a problem hiding this comment.
Done: the test and fix are now isolated in #4053.
|
|
||
| if (!analyzer.Analyze(members)) | ||
| { | ||
| foreach (var constructorDeclaration in members.OfType<ConstructorDeclaration>()) |
There was a problem hiding this comment.
This was intended as a fallback that lifts a base/this constructor call when field-initializer analysis fails. It is too broad and unrelated at this location, so it is excluded from the primary-constructor split. I will only revisit it as a separate narrowly tested fix.
| { | ||
| IType targetTypeHint = constrainedTo ?? memberDeclaringType; | ||
| if (CallInstruction.ExpectedTypeForThisPointer(memberDeclaringType, constrainedTo) == StackType.Ref) | ||
| if (target is Conv { |
There was a problem hiding this comment.
not exactly sure what you are trying to do, but there is a UnwrapConv extension method.
There was a problem hiding this comment.
Thank you. The isolated fix in #4053 now uses target.UnwrapConv(ConversionKind.Invalid), guarded specifically to the synthetic Invalid Ref-to-Unknown conversion on an unresolved value-type receiver. The regression test was run red before the implementation.
Yes, at some point we might add skills and instructions to automate parts of the language feature development process. We have already added a CLAUDE.md to steer LLMs towards correct unit testing of decompiler changes. However, we have repeatedly seen even the most advanced "frontier models" of US providers like Fable 5 stumble into the wrong direction architecture-wise and produce unmaintainable code that only fixes one test case, but does not match the overall architecture/design of the decompiler. That's why careful understanding, thinking and planning before prompting is the responsibility of the human engineer. This is something you cannot just add into CLAUDE.md, since it is not an instruction for the LLM: it's the responsibility of the human engineer. We might add a prompt like:
Fable is great for test case generation though, saves a lot of typing and of course UI/UX development is dead, LLMs are great at that.
Yeah, that exactly IS the problem: In this PR you add at least 3 different decompiler features and hide maybe 2-3 more smaller bug fixes inbetween, that should each be their own PR. I repeat: please split your PR into smaller pieces. Please start with anything other than "collection expressions" since that will need more thinking and planning (on my part). Thank you! |
Currently, we are primarily using Claude, in my experience from work Github Copilot also honors Claude.md and it became the defacto industry standard. Moving to AGENTS.MD is on our list for the future. |
I am currently using codex along with the frontier gpt-5.6-sol model. By default, it doesn't support |
|
Thank you for the detailed feedback. You are right that this PR combines too many independent changes, and my previous reply was too defensive. Sorry about that. I will keep this branch/PR available as a reference, but I will not treat it as a mergeable unit. I am splitting the work from the current upstream
For transparency about the AI-assisted process: there was not one carefully designed initial prompt. The prompts evolved from real decompilation failures, emitted IL from multiple Roslyn versions, the existing ILSpy source/tests, and two large real-world assembly sets. Tests were added during implementation, but I did not consistently write the broad test suite first, and I did not start by agreeing on the ILAst/ExpressionBuilder/C# AST layer or by supplying the collection-expression speclet. I allowed the model to infer the layer from the existing pipeline and did not challenge that choice enough. That was the architectural mistake here, and it is my responsibility as the contributor. For the split work I will follow the repository guidance: spec/design first for language features, red test before implementation, strict pattern matching, full relevant test matrices, and no unrelated fixes hidden in a feature PR. I will post links to the split PRs here as they are ready. Thank you again for taking the time to point out the architectural and reviewability issues. |
|
The first two split PRs are ready:
Both were created from upstream I will not submit the collection-expression implementation as another split PR. The next step for that work will be a design issue first. |
2 participants
Supersedes #3598.
Problem
Recent Visual Studio, Copilot, Semantic Kernel, and OpenAI assemblies expose several compiler-generated patterns that are not fully reconstructed by the current decompiler pipeline. The resulting projects may contain incomplete primary constructors,
.ctor/_002Ectorcalls, synthesized record members, raw backing fields, lowered collection helpers, invalid unresolved value-type call targets, inconsistent project references, or incomplete PDB source mapping.Solution
Primary constructors and constructor initializers
baseandthisconstructor calls instead of emitting.ctoror_002Ector.inarguments.C# 12 collection expressions
CollectionBuildertypes, inline arrays, andCollectionsMarshallowerings.Use collection expressionsoption and run the transform only for C# 12 or later.Collection-expression support is included because modern primary constructors frequently contain collection construction in parameter-dependent initializers and base-constructor arguments.
Records, properties, and unresolved call targets
PrintMembers,Equals,GetHashCode,Deconstruct, copy constructors, and backing-field artifacts when their compiler-generated shapes are proven.Ref -> Unknownconversions and avoid invalid(Type)(ref value)or pointer-based call targets.Project export, assembly resolution, and PDBs
.NETStandarddependencies throughDotNetCorePathFinderand select a coherent closest runtime.Real-world validation
All versions used the same physical DLLs, flattened reference directory, and decompiler settings.
fe70c814858488a4f7b34cfbd9747d5c5a66e78024ec3327e7fa9fcc56159bbbd7cfafdfc3e5ee89e930120ade794c5714e12cc4b9d09e5c8f2f9d98793bced7542ac4bce001473fe285b8749926f94eThe upstream commits between the benchmark baseline and current base modify bundle, resource, and LightJson validation. This branch was rebased onto the current base without conflicts.
Lower is better except for recovered collection-expression counts. Syntax counts are Roslyn parse diagnostics.
Dataset A:
vs2026Assemblies:
Microsoft.VisualStudio.Extensibility.Copilot,Microsoft.VisualStudio.Extensibility.Copilot.Configuration,Microsoft.VisualStudio.LanguageServices.ExternalAccess.Copilot,Microsoft.VisualStudio.Completions,Microsoft.VisualStudio.Completions.Package,Microsoft.VisualStudio.Copilot,Microsoft.VisualStudio.Copilot.Common,Microsoft.VisualStudio.Copilot.Core,Microsoft.VisualStudio.Copilot.Roslyn,Microsoft.VisualStudio.Copilot.UI,Microsoft.VisualStudio.Copilot.UI.Core, andMicrosoft.VisualStudio.Copilot.Vsix..ctor/_002EctorartifactsUnknown result typecomments[CompilerGenerated]markers)(ref ...)patternsThe two remaining
)(ref ...)occurrences are valid delegate invocations. The two remaining syntax diagnostics are inDocument.csand also occur upstream.Dataset B:
Copilot.Conversations.Service_vs2026Assemblies:
Microsoft.SemanticKernel,Microsoft.SemanticKernel.Abstractions,Microsoft.SemanticKernel.Connectors.AzureOpenAI,Microsoft.SemanticKernel.Connectors.Google,Microsoft.SemanticKernel.Connectors.OpenAI,Microsoft.SemanticKernel.Core,Microsoft.VisualStudio.Copilot,Microsoft.VisualStudio.Copilot.Core,Microsoft.VisualStudio.Copilot.Service, andOpenAI..ctor/_002EctorartifactsUnknown result typecomments[CompilerGenerated]markers)(ref ...)patternsReproduction material
The benchmark-only material is stored on a separate branch so it does not affect this PR diff:
No Microsoft binaries, complete decompiled source trees, user names, or user-profile extension entries are included.
Reproduction
Release|Any CPU.Tests
ICSharpCode.Decompiler.Tests: 4,361 total, 0 failed.Review focus
The commits are separated by feature and can be split into smaller follow-up PRs if preferred.