You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Diagnose static data-source properties in inaccessible containing types - #7503
A public static data-source property inside a private nested helper previously emitted references to the inaccessible type and failed with CS0122 in generated code. The generator now checks the full containing-type chain, omits inaccessible initialization, and reports TUNITGEN005 on the user's property with instructions to make the containing types accessible. File-local containers are also excluded.
Regression tests cover private, protected, private protected, internal, protected internal, and public containers, including a public nested type inside each container. They compile generated output, snapshot the diagnostic, and verify that disabling source generation suppresses it. Diagnostics share the initializer discovery pass, use equatable location models, and exclude skipped generic/struct/record helpers. File-local direct/nested and inherited diagnostic deduplication regressions are covered. Reflection initialization remains unchanged: this is validation of references emitted only by the generator.
Validation: core source-generator suite on net10.0: 266 passed, 1 existing skip. Static-property incremental suite on net10.0: 6 passed. No reflection paths or shipped public APIs changed. Other target frameworks are left to CI. Restore's existing Pkcs vulnerability warning is tracked by #7412.
Static data-source properties in inaccessible or file-local types are now reported clearly instead of being silently skipped during initialization generation.
Diagnostics identify the affected property and explain that its containing types must be accessible.
Tests
Added coverage for accessibility scenarios, inherited properties, diagnostic suppression, and source-location accuracy.
No actionable comments were generated in the recent review.
Walkthrough
The static-property generator now detects data-source properties in inaccessible or file-local containing types. It omits those properties from generated initialization and reports TUNITGEN005. Tests cover accessibility, suppression, and diagnostic locations.
Changes: Static Property Accessibility
Layer / File(s)
Summary
Accessibility checks and candidate collection src/TUnit.Core.SourceGenerator/CodeGenerators/StaticPropertyInitializationGenerator.cs, tests/TUnit.Core.SourceGenerator.Tests/StaticPropertyAccessibilityTests.cs, tests/TUnit.Core.SourceGenerator.Tests/StaticPropertyAccessibilityTests.Containing_type_accessibility_accessibility=*.verified.txt
The generator checks containing-type accessibility and records inaccessible properties as diagnostic candidates instead of initialization properties. Tests cover container types, generation options, suppression, and expected diagnostic text.
Diagnostic reporting and location tracking src/TUnit.Core.SourceGenerator/CodeGenerators/StaticPropertyInitializationGenerator.cs, tests/TUnit.Core.SourceGenerator.Tests/StaticPropertyAccessibilityTests.cs
The generator reports deduplicated diagnostics using locations resolved against the current compilation. Tests check diagnostic locations when syntax trees share paths or change order.
No merge-blocking issue is identified; the change is mergeable after normal checks.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title names the main change: the generator now diagnoses static data-source properties whose containing types are inaccessible. The raw summary confirms this via the new TUNITGEN005 diagnostic, sk…
Linked Issues check
Issue [#7461] requires supported initialization or an actionable diagnostic for static data-source properties in inaccessible containing types. The generator checks the full containing-type chain and …
Out of Scope Changes check
The changes stay within issue [#7461]. The generator changes prevent inaccessible references and add the requested diagnostic. The added tests verify accessibility, diagnostic suppression, deduplicati…
Full details: Title check
Explanation
The title names the main change: the generator now diagnoses static data-source properties whose containing types are inaccessible. The raw summary confirms this via the new TUNITGEN005 diagnostic, skipped initialization for inaccessible containers, and accessibility-focused tests. The title is specific and matches the changeset and the linked issue #7461.
Full details: Linked Issues check
Explanation
Issue [#7461] requires supported initialization or an actionable diagnostic for static data-source properties in inaccessible containing types. The generator checks the full containing-type chain and file-local status, omits inaccessible properties from generated initialization, and reports TUNITGEN005 on the property when source generation is enabled. Tests cover private, protected, nested, inherited, file-local, public, and generation-disabled cases.
Full details: Out of Scope Changes check
Explanation
The changes stay within issue [#7461]. The generator changes prevent inaccessible references and add the requested diagnostic. The added tests verify accessibility, diagnostic suppression, deduplication, and incremental syntax-tree behavior for this diagnostic. No unrelated production behavior or public API change is shown.
Full details: Docstring Coverage
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 8 functions across 2 files. (3 skipped: 3 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks each nested type, And marks the ones the code can’t reach. The generator skips those props, Then reports where each problem lies. The test burrow keeps the paths in sight.
CodeRabbit Verify
Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 2bc167d9-fd9a-4b24-8664-07af483b9702
Commits
Reviewing files that changed from the base of the PR and between 9d6a435 and 48937b5.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review.
Correct fix for #7461: the full containing-chain check via IsSymbolAccessibleWithin plus the file-local exclusion is right, and the test matrix covers every accessibility combination and the disabled-generator case. TUNITGEN005 doesn't collide with an existing ID (TUNITGEN004 is the last one used).
1. A Diagnostic is stored in the incremental pipeline.inaccessibleProperties transforms to a Diagnostic? that holds a Location, and therefore a SyntaxTree. Diagnostics use reference equality, so this step never caches and it keeps syntax trees alive between runs. The rest of the pipeline uses EquatableArray models to avoid exactly this. Transform to a small equatable record instead, such as (string PropertyName, string FilePath, TextSpan Span, LinePositionSpan LineSpan), and build the Diagnostic in RegisterSourceOutput. Failing that, wrap it in a record with value equality.
2. Two code paths decide accessibility.IsAccessibleContainingType is called from the new diagnostic provider and from CreateSegment. The diagnostic's property filter (Public getter/setter, data-source attribute) is a hand-copied subset of the segment filter, so the two can drift. Then you get either a silent skip with no diagnostic or a diagnostic with no skip. Sharing one predicate, or deriving the diagnostic from the same segment-building pass, would keep them in sync. It would also avoid a second syntax scan of every static attributed property.
3. Cache keying.cache.GetOrAdd(currentType, CreateSegment(currentType, compilation)) now evaluates CreateSegment eagerly even on a hit, and races can compute it twice. The TryGetValue guard avoids most of this, but a static lambda or a (type, compilation) state overload would be cleaner. Also confirm the cache is per-compilation. If it is static or long-lived, accessibility computed against one Compilation could be reused for another.
4. Minor. The diagnostic message says to make types "internal or public". For a private nested type that means changing the user's design, so a hint that the property can move to an accessible type might help. Consider also adding a test for a generic container and for internal inside private (an inaccessible chain with an accessible leaf).
Overall this is good. Point 1 is worth fixing before merge; the others are optional.
Incremental models now carry an equatable property name, file path, text span, and line span. Diagnostics and locations are built only at source output; no syntax trees are retained in these models.
Diagnostics are derived from the existing class-chain segment pass, using exactly the same discovery and property eligibility checks as initialization. This removes the extra syntax scan and fixes false errors on skipped generic helpers, structs, and records.
Segment creation now uses the lazy GetOrAdd factory after the existing TryGetValue guard. The ConditionalWeakTable is keyed by Compilation, so entries cannot cross compilations; ConcurrentDictionary may still invoke factories concurrently by design.
Added generic-helper and internal-leaf-inside-private-container coverage, including inherited-property diagnostic deduplication. Kept the existing message: making the containing chain accessible remains a valid direct remedy; moving the property is an optional alternative.
Validation: full core generator suite on net10.0: 266 passed, 1 existing skip; static-property incremental suite: 6 passed. Existing diagnostic snapshots pass unchanged. Other frameworks remain for CI. PR remains draft for fresh reviews of this head.
The approach is sound. Reporting a clear diagnostic where the generator used to emit uncompilable code (CS0122) is better than a silent failure. Checking IsSymbolAccessibleWithin over the full containing chain, plus the file-local exclusion, fits the problem. Diagnostics reuse the incremental pipeline with equatable location models, and the tests cover the accessibility matrix, the disabled-generation case and inherited deduplication.
Points to consider:
Duplicate descriptor IDs.TUNITGEN005 is only unique within this assembly. TUNITGEN001 and TUNITGEN002 are each already declared in two different generators (AotConverterGenerator and MethodAssertionGenerator). Another ID reuse of that kind is a real risk, because TUNITGEN004 and TUNITGEN005 live in different projects. I'd put the generator diagnostic IDs in one shared place, or at least a doc table. I also found no docs page or analyzer release-tracking entry for the new ID. Adding one would let users look up what to do when they see it.
Deduplication. The HashSet<InaccessibleProperty> handles the inherited case. Because it is value equality on name, path and span, it is correct. It is rebuilt on every output run over all collected chains, which is fine at this scale. Reporting at the segment level, keyed by TypeKey, would be a little simpler and would avoid the nested loops.
Message accuracy. The message says to make the types "internal or public". A protected internal or public nested type inside an internal outer type also works, so it is slightly over-restrictive. This is minor. Wording like "make the type and its containing types accessible from the assembly" would be more precise.
Silent skip of non-source locations. If a property has no in-source location, for example because it comes from metadata, it is skipped with no diagnostic and no initialization. That seems intended, since such types can't have source-level fixes. A short comment would make that explicit.
Test coverage. The Skipped_helper_has_no_diagnostic cases are good. A case with a generic containing type that wraps an inaccessible nested type would round out the skip logic.
No blocking issues. Looks good to merge once CI is green on the other target frameworks.
Fixed the remaining file-level diagnostic settings finding in d41073e. The incremental discovery models still retain only equatable location data; the output callback uses the current compilation to restore a source-tree location. Added a per-file suppression regression and source-tree identity assertions. Full net10.0 core generator suite: 267 passed, 1 existing skip, existing snapshots unchanged. Other target frameworks remain for CI.
Disposition of the latest Claude suggestions:
Confirmed the existing TUNITGEN001/002 collisions and filed Source generators reuse diagnostic IDs for unrelated rules #7504 for unique generator diagnostic IDs and documentation. TUNITGEN005 is unique; centralizing unrelated generators is outside this fix.
Retained the value-based output deduplication: it correctly handles inherited properties and avoids changing the shared segment pipeline for a non-blocking simplification.
Retained the message: internal/public is a sufficient remedy, though not an exhaustive list of valid accessibility combinations.
No-source properties have nowhere to attach a source diagnostic; the existing source-location guard deliberately omits them.
Additional nested generic coverage is optional; existing open-generic exclusion cases and the accessibility matrix exercise the paths changed here.
PR stays draft pending fresh reviews of this head.
The approach is sound. The generator now checks the whole containing-type chain, drops the initialization that would not compile (CS0122), and reports a clear diagnostic on the user's property. The tests cover the accessibility matrix, file-local types, skipped helpers, deduplication and per-file suppression. I haven't built or run anything.
Points to consider:
Incrementality. The new RegisterSourceOutput combines CompilationProvider with the collected chains. That output step therefore reruns on every compilation change, i.e. every edit. The work is cheap, but the repo's conventions favour keeping it off the hot path. You could emit the diagnostics only when InaccessibleProperties is non-empty, or look the tree up by path in a dictionary instead of scanning SyntaxTrees for each diagnostic. A lighter option is to carry the SyntaxTree reference or a Location only in the output step, and to skip the CompilationProvider combine when no chain has an inaccessible property.
Diagnostic ID collision.TUNITGEN005 is a new ID in TUnit.Core.SourceGenerator. The IDs TUNITGEN001–003 are already used in that project (AotConverterGenerator), and TUNITGEN001–004 in TUnit.Assertions.SourceGenerator. The number space is shared and not unique per generator: the same IDs already mean different things in the two projects. If this ID collides with another descriptor in the assertions generator, release tracking and docs will be ambiguous. Consider a distinct prefix or a central registry. Also confirm that AnalyzerReleases.Unshipped.md or the docs list the new ID, if the repo uses them.
Severity. Error is reasonable, since the code previously failed with CS0122 anyway. Note that a private helper that was only used with reflection-mode initialization now errors under source generation. The message does give the fix, but a Warning might be kinder to users in that situation. This is a judgment call.
Test hygiene. The snapshots only check the message text. The structural assertions in Check already cover the ID and location, so that's fine.
Nothing here blocks the merge. Item 1 is worth a quick look, and item 2 needs a check for collisions.
Fixed duplicate-path diagnostic identity in e39d76c. Full net10.0 core generator suite including snapshots: 269 passed, 1 existing skip; snapshots unchanged. Other target frameworks remain for CI.
Disposition of the latest Claude suggestions: the compilation combine is retained to restore current source-tree identity and per-file options correctly; models remain equatable. TUNITGEN005 is unique and existing cross-generator ID collisions/documentation are tracked in #7504. Error severity is retained because these references previously failed compilation with CS0122; source generation disabled remains covered and emits no diagnostic. Structural diagnostic assertions supplement message snapshots.
PR remains draft pending fresh reviews of the new head.
Review: TUNITGEN005 for inaccessible containing types
The approach is sound. Checking the whole containing-type chain, skipping file-local types, and reporting a clear diagnostic on the user's property is better than emitting code that fails with CS0122. The test matrix is good: every accessibility level, nested types, the disabled-generation case, deduplication, and a suppression case.
Main concern: TreeOrdinal can go stale
InaccessibleProperty stores the syntax tree's index in compilation.SyntaxTrees. The output step then uses SyntaxTrees.ElementAtOrDefault(ordinal) to find the tree again.
The transform only re-runs for syntax nodes that changed. If a new file is added, or the tree order changes, an unchanged file's cached segment keeps its old ordinal.
The ordinal then points at a different tree. The diagnostic lands in the wrong file or at a meaningless span, and pragma and per-file option handling is wrong.
TakeWhile(...).Count() also walks the tree list for every property.
A sturdier option is to keep the tree out of the model and resolve it in the output step from the file path plus the span. To distinguish duplicate paths, use something stable such as the text content hash. Alternatively, report the diagnostic from its own RegisterSourceOutput over a syntax-provider result that carries the node's location, with no CompilationProvider combine.
Smaller points
.Combine(context.CompilationProvider) forces this output to re-run on every compilation change. The cost is small, but it works against the incremental design the surrounding code is careful about.
Add a test that inserts an unrelated syntax tree before the offending one on a second run. That is exactly the stale-ordinal scenario above, and the duplicate-path test does not cover it.
The message says to make types "internal or public". For file types the right advice is to remove the file modifier, so consider a separate message for that case.
HasAnyProperty now also counts diagnostics, so a name like HasAnyEntry would be clearer.
Overall: good direction. I'd fix the ordinal fragility before merging.
Addressed the latest review with regression coverage in 48937b5. The stale-ordinal concern does not reproduce: CreateSyntaxProvider's semantic transform reruns when the compilation changes, and SegmentCaches is keyed by Compilation, so cached segments from the earlier compilation are not reused. Added second-run tests reusing the generator driver and unchanged offending syntax tree, both inserting a new tree before it and reordering existing trees. All trees deliberately share a path; suppression applies only to the unrelated tree. Both cases preserve the diagnostic's exact SourceTree, property span, and file options. Added a source comment documenting this invariant.
Validation: full net10.0 core generator suite including snapshots passed: 271 succeeded, 1 existing skip, 0 failures. No snapshot changes. Other target frameworks remain for CI.
Smaller suggestions: retain the compilation combine to attach diagnostics to current trees and honor file options. Retain the existing accessibility message, which gives a sufficient remedy (replace file with internal/public for file-local types). Retain HasAnyProperty to avoid unrelated naming churn. Prior diagnostic-ID/documentation follow-up remains tracked by #7504.
The PR remains draft pending fresh Claude and Codex reviews of this head.
The fix is correct in direction. It checks the whole containing-type chain, treats file-local types as inaccessible, skips the emitted initialization, and reports a diagnostic on the user's property instead of letting generated code fail with CS0122. The tests are thorough: all accessibility combinations, file-local types, skipped generic/struct/record helpers, inherited-chain deduplication, and duplicate tree paths.
I read the diff only and did not build or run the tests. One design concern and two smaller points:
TreeOrdinal in the model works against incrementality.
Each InaccessibleProperty stores the tree's index in compilation.SyntaxTrees, computed with an O(n) TakeWhile(...).Count(). The output step then calls SyntaxTrees.ElementAtOrDefault(ordinal) to rebuild the location.
Adding or reordering any file earlier in the compilation changes the ordinal, so the cached segment is no longer equal. The long comment explains why this is needed, but it is a lot of machinery to keep a Location alive.
The output step also combines with context.CompilationProvider, so it reruns on every compilation change (every edit in the IDE). It is cheap, but it is the pattern Roslyn incremental-generator guidance warns against.
A simpler option is to emit this diagnostic from a companion DiagnosticAnalyzer (symbol-action on static properties with a data-source attribute). Analyzers get real Locations, pragma handling and .editorconfig severity without caching hacks. The generator would only need the accessibility check to skip emission. The repo already ships analyzers, and it would keep diagnostics out of the generator's cached models.
If the diagnostic stays in the generator, consider carrying a Location through a small wrapper, or at least keep the approach as is and document that this is the accepted tradeoff.
The accessibility logic is duplicated.IsAccessibleContainingType only partly overlaps the accessibility filters used elsewhere in the generators. Check whether a shared helper already exists, so test generation and static-property initialization cannot drift apart.
Message wording. "Make the type and all containing types internal or public" is slightly misleading for file-local types, where the fix is to remove file. Consider mentioning that.
Overall this is a solid fix with strong regression coverage. I'd recommend considering the analyzer-based approach for the diagnostic before merging, but I don't see a correctness blocker.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
A public static data-source property inside a private nested helper previously emitted references to the inaccessible type and failed with CS0122 in generated code. The generator now checks the full containing-type chain, omits inaccessible initialization, and reports TUNITGEN005 on the user's property with instructions to make the containing types accessible. File-local containers are also excluded.
Regression tests cover private, protected, private protected, internal, protected internal, and public containers, including a public nested type inside each container. They compile generated output, snapshot the diagnostic, and verify that disabling source generation suppresses it. Diagnostics share the initializer discovery pass, use equatable location models, and exclude skipped generic/struct/record helpers. File-local direct/nested and inherited diagnostic deduplication regressions are covered. Reflection initialization remains unchanged: this is validation of references emitted only by the generator.
Validation: core source-generator suite on net10.0: 266 passed, 1 existing skip. Static-property incremental suite on net10.0: 6 passed. No reflection paths or shipped public APIs changed. Other target frameworks are left to CI. Restore's existing Pkcs vulnerability warning is tracked by #7412.
Closes #7461
Summary by CodeRabbit