feat: Phase 2 constructor + accessor-method migrations#7
Merged
Conversation
Extend the analyzer with one discriminator per supported pattern and dispatch each
to a dedicated code-fix rewrite. All patterns share `TestablyAbstractionsMigration001`.
Constructors
- `new MockFileSystem(MockFileSystemOptions { CurrentDirectory = "x" })` folds into
`new MockFileSystem(o => o.UseCurrentDirectory("x"))`. Empty initializer collapses
to the parameterless form. Properties without a Testably equivalent
(`CreateDefaultTempDir`, etc.) emit no fix.
- `new MockFileSystem(dictLiteral[, "currentDir"])` and `new MockFileSystem(
dictLiteral, optionsLiteral)` expand at the call site when assigned to a single
local in a block: the constructor becomes parameterless / options-only, and one
`fs.File.WriteAllText(...)` / `WriteAllBytes(...)` statement is appended per
dictionary entry. Each entry is classified semantically (string vs encoded vs
bytes); any unsupported `MockFileData` shape disables the whole expansion.
1:1 accessor methods (IMockFileDataAccessor surface)
- `AddDirectory` → `Directory.CreateDirectory`
- `RemoveFile` → `File.Delete` (drops `verifyAccess`)
- `MoveDirectory`→ `Directory.Move`
- `FileExists` → `File.Exists`
- `AddEmptyFile` → `File.Create(p).Dispose()`
- `AddFile` → `File.WriteAllText[, encoding]` or `WriteAllBytes`, dispatched
on the `MockFileData` constructor overload via the semantic model. Non-literal
data or initializer-with-properties cases register no fix.
Implementation notes
- Follow-up statements for the dictionary expansion are produced via
`ParseStatement(text)` rather than `SyntaxFactory.ExpressionStatement`. The
latter emits elastic trivia that the Formatter normalizes alongside the
enclosing block's closing brace, breaking the source's indentation style.
- The playground accumulates one sample per Phase 2 pattern so the parity check
exercises the full fixer dispatch table.
Test Results 6 files ± 0 6 suites ±0 37s ⏱️ -2s Results for commit a0265b2. ± Comparison against base commit ca49479. This pull request removes 1 and adds 30 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
This PR expands the System.IO.Abstractions migration analyzer/code fix to cover Phase 2 constructor and accessor-method patterns, adding diagnostics and targeted rewrites toward Testably.Abstractions.Testing equivalents.
Changes:
- Added analyzer pattern classification for MockFileSystem option/file constructors and accessor methods.
- Added code-fix rewrites for supported constructors, options lambdas, dictionary expansion, and accessor method replacements.
- Added tests and playground samples for the new Phase 2 migration scenarios.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
Source/Testably.Abstractions.Migration.Analyzers/SystemIOAbstractionsAnalyzer.cs |
Adds invocation analysis and constructor/accessor pattern classification. |
Source/Testably.Abstractions.Migration.Analyzers/Patterns.cs |
Defines new diagnostic pattern discriminator constants. |
Source/Testably.Abstractions.Migration.Analyzers.CodeFixers/SystemIOAbstractionsCodeFixProvider.cs |
Implements dispatch and rewrites for the new constructor/accessor patterns. |
Tests/Testably.Abstractions.Migration.Tests/SystemIOAbstractionsAnalyzerTests.cs |
Adds analyzer coverage for Phase 2 constructor and accessor diagnostics. |
Tests/Testably.Abstractions.Migration.Tests/SystemIOAbstractionsCodeFixProviderTests.cs |
Adds code-fix coverage for supported and unsupported Phase 2 rewrites. |
Tests/Testably.Abstractions.Migration.SystemIOAbstractionsPlayground/MockFileSystemSamples.cs |
Adds playground examples for the newly supported migration patterns. |
Comments suppressed due to low confidence (3)
Source/Testably.Abstractions.Migration.Analyzers.CodeFixers/SystemIOAbstractionsCodeFixProvider.cs:662
- This uses the same fixed lambda parameter name for the string-current-directory constructor rewrite, so a source expression that references a variable named
ois captured incorrectly after the fix (new MockFileSystem(files, o)becomeso => o.UseCurrentDirectory(o)). Generate a non-conflicting parameter name before wrapping the expression in the lambda.
SimpleLambdaExpressionSyntax lambda = SyntaxFactory.SimpleLambdaExpression(
SyntaxFactory.Parameter(SyntaxFactory.Identifier("o")),
SyntaxFactory.InvocationExpression(
SyntaxFactory.MemberAccessExpression(
SyntaxKind.SimpleMemberAccessExpression,
SyntaxFactory.IdentifierName("o"),
SyntaxFactory.IdentifierName("UseCurrentDirectory")),
SyntaxFactory.ArgumentList(SyntaxFactory.SingletonSeparatedList(
SyntaxFactory.Argument(expression.WithoutTrivia())))));
Source/Testably.Abstractions.Migration.Analyzers.CodeFixers/SystemIOAbstractionsCodeFixProvider.cs:586
- The files-constructor rewrite also leaves the original creation type unchanged and depends on the using directive swap to retarget it. For alias-qualified or fully qualified
System.IO.Abstractions.TestingHelpers.MockFileSystemcreations, the old type remains while the arguments are rewritten to Testably's constructor shape, so the code fix can emit code that does not compile.
BaseObjectCreationExpressionSyntax newCreation = creation! switch
{
ObjectCreationExpressionSyntax oc => oc
.WithArgumentList(newArgList.WithTriviaFrom(argList))
.WithInitializer(null),
ImplicitObjectCreationExpressionSyntax ic => ic
.WithArgumentList(newArgList.WithTriviaFrom(argList))
.WithInitializer(null),
_ => creation!,
Source/Testably.Abstractions.Migration.Analyzers.CodeFixers/SystemIOAbstractionsCodeFixProvider.cs:401
AddFilehas the same interface-receiver problem: diagnostics are produced forIMockFileDataAccessor, but the replacement unconditionally targets<receiver>.File.WriteAllText/Bytes. AnIMockFileDataAccessorvariable does not exposeFile, so the registered code fix can generate uncompilable code unless it first verifies or rewrites the receiver type.
MemberAccessExpressionSyntax newAccess = SyntaxFactory.MemberAccessExpression(
SyntaxKind.SimpleMemberAccessExpression,
SyntaxFactory.MemberAccessExpression(
SyntaxKind.SimpleMemberAccessExpression,
memberAccess.Expression,
SyntaxFactory.IdentifierName("File")),
SyntaxFactory.IdentifierName(newMethod));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
vbreuss
force-pushed
the
feat/phase-2-constructor-and-accessor-migrations
branch
from
db7c930 to
a0265b2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
Source/Testably.Abstractions.Migration.Analyzers.CodeFixers/SystemIOAbstractionsCodeFixProvider.cs:372
- This rewrite assumes the original argument order is the target method order after stripping name colons. Calls with named arguments out of positional order, such as
MoveDirectory(destPath: "/b", sourcePath: "/a")orRemoveFile(verifyAccess: false, path: "/a"), are valid C# but would be rewritten with swapped or wrong arguments (forRemoveFile,File.Delete(false)). The fix should either map arguments by parameter name before normalizing or decline to register for out-of-order named arguments.
SeparatedSyntaxList<ArgumentSyntax> args = original.ArgumentList.Arguments;
int keep = argCountToKeep < args.Count ? argCountToKeep : args.Count;
// Strip the NameColon: TestableIO and Testably use different parameter names
// (e.g. `MoveDirectory(sourcePath:, destPath:)` vs `Directory.Move(sourceDirName:,
// destDirName:)`). Positional binding is the only safe form across the swap.
SeparatedSyntaxList<ArgumentSyntax> normalized = SyntaxFactory.SeparatedList(
args.Take(keep).Select(arg => arg.WithNameColon(null)));
Source/Testably.Abstractions.Migration.Analyzers.CodeFixers/SystemIOAbstractionsCodeFixProvider.cs:134
- The options-constructor fix only looks for
ObjectCreationExpressionSyntax, so target-typed constructions likeMockFileSystem fs = new(new MockFileSystemOptions { CurrentDirectory = "/work" });are flagged by the analyzer but never get this rewrite. Other constructor paths in this file useBaseObjectCreationExpressionSyntax, so this should handleImplicitObjectCreationExpressionSyntaxas well or explicitly suppress registration for that syntax.
private static void TryRegisterOptionsCtorFix(CodeFixContext context, Diagnostic diagnostic, SyntaxNode node)
{
ObjectCreationExpressionSyntax? creation = node.FirstAncestorOrSelf<ObjectCreationExpressionSyntax>();
if (creation?.ArgumentList is not { Arguments.Count: 1, } argList
|| !HasUnqualifiedMockFileSystemTypeName(creation))
Source/Testably.Abstractions.Migration.Analyzers.CodeFixers/SystemIOAbstractionsCodeFixProvider.cs:488
- This constructs a fresh separated argument list without preserving or supplying comma trivia, so the generated call is formatted as
WriteAllText("/foo","hello")(and similarly for the encoding overload) unless a formatter is explicitly run. The added tests and surrounding code style expect a space after commas; reuse the original separators where possible or add formatter/elastic trivia to avoid producing poorly formatted fixes.
SeparatedSyntaxList<ArgumentSyntax> args = SyntaxFactory.SeparatedList(new[]
{
pathArg.WithNameColon(null).WithoutTrivia(),
SyntaxFactory.Argument(shape.PrimaryContent.WithoutTrivia()),
});
if (shape.SecondaryContent is not null)
{
args = args.Add(SyntaxFactory.Argument(shape.SecondaryContent.WithoutTrivia()));
Source/Testably.Abstractions.Migration.Analyzers.CodeFixers/SystemIOAbstractionsCodeFixProvider.cs:237
- This helper blindly moves the provided expression into a synchronous options lambda. If
CurrentDirectoryor the string constructor argument containsawait, the original code is valid inside an async method, but the rewritteno => o.UseCurrentDirectory(await ...)will not compile. Reject RHS expressions that cannot legally be embedded in a non-async lambda before registering either constructor fix.
private static SimpleLambdaExpressionSyntax BuildUseCurrentDirectoryLambda(ExpressionSyntax currentDirectory)
{
// Avoid shadowing identifiers used inside the captured `currentDirectory`
// expression. `new MockFileSystemOptions { CurrentDirectory = o }` must not
// rewrite to `o => o.UseCurrentDirectory(o)`.
string parameterName = PickFreshLambdaParameterName(currentDirectory);
`HasUnqualifiedMockFileSystemTypeName` previously approved every `ImplicitObjectCreationExpressionSyntax`. But `new()` is target-typed: an enclosing fully-qualified or alias-qualified context (`System.IO.Abstractions.TestingHelpers.MockFileSystem fs = new();`) keeps the construction bound to TestableIO even after the using swap, so the fix would leave the source half-rewritten. Narrow the implicit case to the one shape we can verify is safe — the target type annotation on the enclosing variable declaration is itself `IdentifierNameSyntax`. Other target-typing contexts (parameters, returns, assignments to non-local LHS, casts) fall through to manual review. Add a regression test pinning the qualified-LHS no-fix path.
|
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.
Extend the analyzer with one discriminator per supported pattern and dispatch each to a dedicated code-fix rewrite. All patterns share
TestablyAbstractionsMigration001.Constructors
new MockFileSystem(MockFileSystemOptions { CurrentDirectory = "x" })folds intonew MockFileSystem(o => o.UseCurrentDirectory("x")). Empty initializer collapses to the parameterless form. Properties without a Testably equivalent (CreateDefaultTempDir, etc.) emit no fix.new MockFileSystem(dictLiteral[, "currentDir"])andnew MockFileSystem( dictLiteral, optionsLiteral)expand at the call site when assigned to a single local in a block: the constructor becomes parameterless / options-only, and onefs.File.WriteAllText(...)/WriteAllBytes(...)statement is appended per dictionary entry. Each entry is classified semantically (string vs encoded vs bytes); any unsupportedMockFileDatashape disables the whole expansion.1:1 accessor methods (IMockFileDataAccessor surface)
AddDirectory→Directory.CreateDirectoryRemoveFile→File.Delete(dropsverifyAccess)MoveDirectory→Directory.MoveFileExists→File.ExistsAddEmptyFile→File.Create(p).Dispose()AddFile→File.WriteAllText[, encoding]orWriteAllBytes, dispatchedon the
MockFileDataconstructor overload via the semantic model. Non-literaldata or initializer-with-properties cases register no fix.