Generate DecompilerSettings boilerplate from [DecompilerSetting] attributes - #3970
Conversation
christophwille
left a comment
There was a problem hiding this comment.
Code review (high effort, multi-agent)
The review of the DecompilerSettings source-generator migration surfaced two real generator robustness bugs: unguarded null-forgiving casts on error-typed attribute arguments crash the entire generator (flooding the build with cascading partial-property errors instead of one targeted diagnostic), and the AddSource hint name drops the namespace half of the (Namespace, ClassName) grouping key, so same-named settings classes in different namespaces would collide deterministically.
The PR also folds two disclosed but untested behavior changes into the refactor: SwitchOnReadOnlySpanChar is newly gated on C# 11, and ExtensionMethods/UseLambdaSyntax/UseEnhancedUsing now raise GetMinimumRequiredVersion -- both of which can change decompiled output or make WholeProjectDecompiler.LanguageVersion assignment throw where it previously succeeded. The 333-line generator with three diagnostics also ships with zero tests.
Remaining findings are cleanups: DSTG003 double-reports via a redundant CS0579, a duplicated EmbeddedAttribute post-init declaration, alias-ambiguous enum-name recovery in diagnostics, and a catch-all diagnostic-id switch that would misreport future ids.
9 findings posted as inline comments (4 correctness, 1 test coverage, 4 cleanup). 16 candidates were independently verified; 15 survived, merged into these 9.
Assisted-by: Claude:claude-fable-5:Claude Code
405bda0 to
f7e6a8a
Compare
christophwille
left a comment
There was a problem hiding this comment.
Re-review (high effort, multi-agent)
Follow-up to the previous review, against head f7e6a8a.
Six of the nine previous findings are fixed at this head: error-typed attribute arguments are now guarded (skip instead of crash), the hint name carries the namespace, DSTG003 suppresses the generated [Category], the EmbeddedAttribute source is shared via RoslynHelpers, the enum name is recovered from syntax (alias-safe), and the diagnostic-id lookup is a fail-loud dictionary. Nice turnaround.
Three findings carry over: the two behavior changes (ExtensionMethods/UseLambdaSyntax/UseEnhancedUsing raising the minimum version; SwitchOnReadOnlySpanChar gated on C# 11) are now explicit, well-described commits -- but both still ship without a regression test pinning the new semantics, and the generator itself remains untested.
Five new findings: two latent generator input-validation gaps (invalid output for global-namespace classes and for camelCase-named settings), a missing containing-type check, duplicated LanguageVersion validation in WholeProjectDecompiler, and 14 handwritten C# 1.0 [Category] literals the generator's category map could single-source.
8 findings posted as inline comments (4 correctness, 1 test coverage, 3 cleanup). 18 candidates were independently verified; the one candidate targeting the since-removed AffectsMinimumRequiredVersion knob was dropped as obsolete (the branch moved mid-review).
Assisted-by: Claude:claude-fable-5:Claude Code
…ibutes Every version-gated setting was bookkept in four places that had to stay in sync by hand: the property boilerplate, SetLanguageVersion, GetMinimumRequiredVersion, and the [Category] display string - and that sync had already drifted in a handful of settings. A new source generator in ICSharpCode.Decompiler.Generators now derives all four from a single [DecompilerSetting] attribute on a partial property: backing field, accessors with change notification, the version-derived [Category], and both version methods. [Description] stays handwritten because its resource keys are irregular and are grepped from the resx. This commit is a 1:1 translation: the current inconsistencies are reproduced exactly (AffectsMinimumRequiredVersion = false on ExtensionMethods, UseLambdaSyntax and UseEnhancedUsing; no gate on SwitchOnReadOnlySpanChar), verified against the old build by comparing SetLanguageVersion and GetMinimumRequiredVersion behavior for every setting at every language version, plus a reflection diff of the full per-property attribute surface. Assisted-by: Claude:claude-fable-5:Claude Code
…minimum version These three settings were disabled by SetLanguageVersion for older targets but, unlike every comparable syntax-preference setting, never raised GetMinimumRequiredVersion while enabled - an omission that had gone unnoticed in the handwritten version bookkeeping. Drop the AffectsMinimumRequiredVersion escape hatch that reproduced it. Assisted-by: Claude:claude-fable-5:Claude Code
The setting carried the C# 11.0 display category but was missing from both SetLanguageVersion and GetMinimumRequiredVersion, so decompiling for an older target language version could still produce switches over ReadOnlySpan<char> that the requested compiler cannot compile. Gating it like the other C# 11.0 settings closes that gap; the category string is now derived from the version like everywhere else. Assisted-by: Claude:claude-fable-5:Claude Code
Four settings used the bare category string "Other" while the rest of the group uses the "DecompilerSettings.Other" resource key. Both happen to resolve to the same English text today, so the options UI shows one group, but the two keys would split into separate groups the moment their translations diverge. Assisted-by: Claude:claude-fable-5:Claude Code
The LanguageVersion setter's InvalidOperationException is a safety net against exporting a project whose LangVersion cannot compile the emitted code, but it only fires at assignment time: Settings is mutable and shared, so enabling a feature after assigning the version slipped past the check. Re-validating at the start of DecompileProject closes that gap while keeping the setter's immediate feedback. Assisted-by: Claude:claude-fable-5:Claude Code
The language version appears in two places that share a name but not a concept, which repeatedly reads as one confused API: on DecompilerSettings it is a construction shortcut (SetLanguageVersion initializes the feature flags once and the version is not stored, so the flags are the only state and the call is deliberately one-way), while on WholeProjectDecompiler it is an export parameter (the LangVersion stamped into the project file, defaulting to GetMinimumRequiredVersion() and rejected below it as a safety net against exporting uncompilable projects). Spell both roles out in the XML docs so the distinction no longer has to be reverse-engineered. Assisted-by: Claude:claude-fable-5:Claude Code
The 14 C# 1.0 settings each carried a handwritten
[Category("C# 1.0 / VS .NET")] literal, duplicating the per-version
display knowledge the generator's CategoryByVersion map single-sources.
Gating them on LanguageVersion.CSharp1 instead is observably identical:
CSharp1 is the smallest enum value, so the generated SetLanguageVersion
bucket can never fire, and the new GetMinimumRequiredVersion arm returns
the same CSharp1 the final fallback already does.
Assisted-by: Claude:claude-fable-5:Claude Code
Structural generator mistakes surface at compile time via DSTG002-005 and partial-member matching, and emission regressions light up the fixture suite - except one: dropping the reversed bucket scan in the generated GetMinimumRequiredVersion compiles green and returns the lowest enabled feature version instead of the highest, and the method's only consumer is project-export LangVersion stamping, which default CI runs barely exercise. Pin the highest-wins contract, including the syntax-preference settings that now participate in the ladder. Assisted-by: Claude:claude-fable-5:Claude Code
f7e6a8a to
7a7cb44
Compare
Every version-gated decompiler setting was bookkept in four places that had to stay in sync by hand: the property boilerplate,
SetLanguageVersion,GetMinimumRequiredVersion, and the[Category]display string. A survey of the file showed that sync had already drifted in four settings.This PR adds a second source generator to
ICSharpCode.Decompiler.Generatorsthat derives all four from a single[DecompilerSetting]attribute on a partial property: the backing field, the accessors with change notification, the version-derived[Category], and both version methods.DecompilerSettings.csshrinks from ~2450 to ~990 lines, and adding a new setting is now ~5 lines in one place:[Description]stays handwritten: 51 of the 121 resource keys do not follow theDecompilerSettings.{PropertyName}convention, and the keys are what you grep for from the resx. Settings without a version argument (VB/F#/UI options) keep their handwritten[Category]. New diagnostics keep the scheme honest: DSTG002 (attribute target must be a partial instance bool property), DSTG003 (version-gated settings must not also declare a handwritten[Category], which would makeGetCustomAttribute<CategoryAttribute>()throw), DSTG004 (a newLanguageVersionneeds a display-category mapping in the generator), DSTG005 (the containing class must be a non-nested partial class).The first commit is a strict 1:1 translation - the pre-existing inconsistencies are reproduced exactly. The three follow-up commits then remove them, each independently droppable:
ExtensionMethods,UseLambdaSyntaxandUseEnhancedUsingwere disabled bySetLanguageVersionfor old targets but never raisedGetMinimumRequiredVersionwhile enabled, unlike every comparable syntax-preference setting.SwitchOnReadOnlySpanCharcarried the C# 11.0 category but was missing from both version methods, so decompiling for an older target could still produce switches overReadOnlySpan<char>that the requested compiler cannot compile."Other"instead of theDecompilerSettings.Otherresource key; both currently resolve to the same English text, but the keys would split into two options groups the moment their translations diverge.Verification: a throwaway characterization harness recorded, on the old code, the full property vector after
SetLanguageVersionfor everyLanguageVersionand theGetMinimumRequiredVersionresult for each setting toggled alone, and was re-run green against the generated code (red/green around each behavior-changing commit); a reflection diff of the per-propertyCategory/Description/Browsablesurface between the old and new build came back identical for all 122 bool properties (also proving the merged partial-property attributes stay unambiguous for the options UI and settings XML round-trip). Full decompiler suite (3365 tests) and ILSpy UI suite (1178 tests) pass.Later commits from the review rounds re-validate an explicitly set
LanguageVersionwhen project export starts, document the two roles the language version plays in the settings API, and derive the C# 1.0[Category]strings from the generator's version map as well (observably identical:CSharp1is the enum floor, so the new cascade bucket cannot fire and the new minimum-version arm returns what the fallback already did).Identified but out of scope: with
UseLambdaSyntaxenabled, statement-bodied anonymous functions still decompile todelegatesyntax; extending the setting to emit statement lambdas is a candidate follow-up.This PR description was written by the Claude Code agent session that authored the change.
🤖 Generated with Claude Code