feat: Phase 4c discriminate captured-reference property accesses#12
Merged
Conversation
Today every MockFileData property access reports the generic propertyRead or propertyWrite pattern; the code-fix's TryMatchOneShotGetFileRead/Write then bails internally for captured references (locals, parameters, fields) because the receiver is not an `<expr>.GetFile(path)` invocation. That works but the diagnostic is indistinguishable from a fixable one-shot access — users have no signal that flow analysis is what's missing. Move the shape check into the analyzer: a property reference whose Instance is `<expr>.GetFile(<single-arg>)` stays on the existing propertyRead/Write pattern; everything else gets the new capturedReferenceRead/Write pattern. The code-fix dispatcher has no case for the captured-ref patterns, so it falls through silently — observable behavior is unchanged. The code-fix's internal shape check is kept for defense in depth: if the analyzer's gate is ever loosened, the code-fix still won't try to rewrite a non-one-shot site. Tests cover local, parameter and field receivers; the existing CapturedReference HasNoFix tests still pass via the new pattern fall- through and get an updated comment.
There was a problem hiding this comment.
Pull request overview
This PR refines the System.IO.Abstractions migration analyzer to distinguish one-shot fs.GetFile(path).Prop property accesses (potentially auto-fixable) from captured-reference MockFileData property accesses (manual review), improving diagnostic signal without changing observable code-fix behavior.
Changes:
- Update the analyzer to emit new pattern IDs for
MockFileDataproperty reads/writes when the receiver is not a one-shotGetFile(...)invocation. - Add analyzer tests covering captured-reference receivers via local, parameter, and field.
- Update/extend test fixtures and comments to reflect the new Phase 4c classification.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Tests/Testably.Abstractions.Migration.Tests/SystemIOAbstractionsCodeFixProviderTests.MockFileDataTests.cs | Updates the captured-reference “no fix” test commentary to reflect the new analyzer pattern classification. |
| Tests/Testably.Abstractions.Migration.Tests/SystemIOAbstractionsAnalyzerTests.cs | Adds new analyzer tests for captured-reference reads/writes (local/parameter/field). |
| Tests/Testably.Abstractions.Migration.SystemIOAbstractionsPlayground/MockFileDataTests.cs | Adds a playground/manual-review fixture demonstrating captured-reference access via a local. |
| Source/Testably.Abstractions.Migration.Analyzers/SystemIOAbstractionsAnalyzer.cs | Implements Phase 4c discriminator logic to choose between one-shot vs captured-reference pattern IDs. |
| Source/Testably.Abstractions.Migration.Analyzers/Patterns.cs | Introduces new pattern constants for captured-reference reads and writes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Comment on lines
+322
to
+362
| [Fact] | ||
| public async Task MockFileDataCapturedReferenceRead_FromLocal_ShouldBeFlagged() | ||
| { | ||
| const string source = """ | ||
| using System.IO.Abstractions.TestingHelpers; | ||
|
|
||
| public class C | ||
| { | ||
| public string Read(MockFileSystem fs) | ||
| { | ||
| MockFileData data = fs.GetFile("/a"); | ||
| return {|#0:data.TextContents|}; | ||
| } | ||
| } | ||
| """; | ||
|
|
||
| await Verifier.VerifyAnalyzerAsync( | ||
| source, | ||
| Verifier.Diagnostic(Rules.SystemIOAbstractionsRule).WithLocation(0)); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task MockFileDataCapturedReferenceWrite_FromLocal_ShouldBeFlagged() | ||
| { | ||
| const string source = """ | ||
| using System.IO; | ||
| using System.IO.Abstractions.TestingHelpers; | ||
|
|
||
| public class C | ||
| { | ||
| public void Write(MockFileSystem fs) | ||
| { | ||
| MockFileData data = fs.GetFile("/a"); | ||
| {|#0:data.Attributes|} = FileAttributes.ReadOnly; | ||
| } | ||
| } | ||
| """; | ||
|
|
||
| await Verifier.VerifyAnalyzerAsync( | ||
| source, | ||
| Verifier.Diagnostic(Rules.SystemIOAbstractionsRule).WithLocation(0)); |
Comment on lines
+94
to
+104
| /// A read access to a <c>MockFileData</c> property whose receiver is a captured | ||
| /// reference (local, parameter, field, etc.) rather than a one-shot | ||
| /// <c>fs.GetFile(path)</c> invocation — no safe textual rewrite without flow | ||
| /// analysis. | ||
| /// </summary> | ||
| public const string MockFileDataCapturedReferenceRead = "MockFileData.capturedReferenceRead"; | ||
|
|
||
| /// <summary> | ||
| /// A write/assignment to a <c>MockFileData</c> property whose receiver is a | ||
| /// captured reference rather than a one-shot <c>fs.GetFile(path)</c> invocation | ||
| /// — no safe textual rewrite without flow analysis. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Today every MockFileData property access reports the generic propertyRead or propertyWrite pattern; the code-fix's TryMatchOneShotGetFileRead/Write then bails internally for captured references (locals, parameters, fields) because the receiver is not an
<expr>.GetFile(path)invocation. That works but the diagnostic is indistinguishable from a fixable one-shot access — users have no signal that flow analysis is what's missing.Move the shape check into the analyzer: a property reference whose Instance is
<expr>.GetFile(<single-arg>)stays on the existing propertyRead/Write pattern; everything else gets the new capturedReferenceRead/Write pattern. The code-fix dispatcher has no case for the captured-ref patterns, so it falls through silently — observable behavior is unchanged.The code-fix's internal shape check is kept for defense in depth: if the analyzer's gate is ever loosened, the code-fix still won't try to rewrite a non-one-shot site.
Tests cover local, parameter and field receivers; the existing CapturedReference HasNoFix tests still pass via the new pattern fall- through and get an updated comment.