From b2a75b26b4a8e7ede4ec9895b5f0fec84ca5cba0 Mon Sep 17 00:00:00 2001 From: Almar Aubel Date: Wed, 3 Jul 2024 11:16:29 +0200 Subject: [PATCH] Fix pattern matching bugs in 1.4.1 (#43) * Fix pattern matching bugs in 1.4.1 Addressed multiple issues related to pattern matching and complex conditional logic that were impacting code analysis accuracy. This update ensures more reliable validation of advanced C# constructs. * Refactor code and add more test cases The code has been refactored to remove redundancies, fix incorrect logic, and improve readability. In addition, multiple test cases have been added to verify the absence of diagnostics when accessing a value after checking for failure. * Refine logic in WalkerResult The ternary condition inside the CorrectUsage property in WalkerResult.cs has been adjusted. The change ensures that the property correctly combines conditions for CheckedFailure, termination, and non-use of "AccessedValue". --- ...ctionalExtensions.Analyzers.Samples.csproj | 2 +- .../Class1.cs | 38 ++++++++++-- ...unctionalExtensions.Analyzers.Tests.csproj | 2 +- .../UseResultValueWithoutCheckTests.cs | 58 ++++++++++++++++++- ...SharpFunctionalExtensions.Analyzers.csproj | 2 +- .../ResultValueWalker.cs | 43 +++++++++++--- README.md | 2 +- build/Build.cs | 17 +++--- changelog.md | 2 + 9 files changed, 140 insertions(+), 26 deletions(-) diff --git a/CSharpFunctionalExtensions.Analyzers.Samples/CSharpFunctionalExtensions.Analyzers.Samples.csproj b/CSharpFunctionalExtensions.Analyzers.Samples/CSharpFunctionalExtensions.Analyzers.Samples.csproj index 8c7870b..d068567 100644 --- a/CSharpFunctionalExtensions.Analyzers.Samples/CSharpFunctionalExtensions.Analyzers.Samples.csproj +++ b/CSharpFunctionalExtensions.Analyzers.Samples/CSharpFunctionalExtensions.Analyzers.Samples.csproj @@ -15,7 +15,7 @@ OutputItemType="Analyzer" ReferenceOutputAssembly="false"/> - + diff --git a/CSharpFunctionalExtensions.Analyzers.Samples/Class1.cs b/CSharpFunctionalExtensions.Analyzers.Samples/Class1.cs index 0603f6c..3c89b88 100644 --- a/CSharpFunctionalExtensions.Analyzers.Samples/Class1.cs +++ b/CSharpFunctionalExtensions.Analyzers.Samples/Class1.cs @@ -44,22 +44,29 @@ public int Test() { var y = Result.Success(1); - if (y.IsFailure) return y.Value; - + if (!y.IsSuccess || y.Value > 0 ) return 0; + return 1; } + public int Test2() + { + var y = Result.Success(1); + var x = y.IsSuccess ? y.Value : 0; + var foo = y.Value; + return y.IsFailure ? 0 : y.Value; + } public int PatternMatching() { var result = Result.Success(1); var id = result switch { - { IsSuccess: true }=> result.Value, - { Error: "eror" } => 0, + { IsSuccess: true, Value: 1 }=> result.Value, + { Error: "eror", Value: 1 } => 0, _ => throw new ArgumentOutOfRangeException() }; - var x = (result.IsFailure)? 0: result.Value; + var x = result.IsFailure? 0: result.Value; switch (result.IsSuccess) { case true: @@ -75,10 +82,29 @@ public void UsingStatementExample() { var result = Result.Success(1); if (result.IsFailure) return; - + //var resultMessage = !result.IsSuccess ? $"{result.Value} success." : "Failed."; using (var streamWriter = new StreamWriter("filePath")) { streamWriter.Write(result.Value); } } + + public void CombinedCheckExample() + { + var result = Result.Success(1); + //if (result.IsFailure || result.Value == 1 ) return; + if (result is { IsSuccess: true, Value: > 1 }) + { + Console.WriteLine("foo" + result.Value); + } + + // if (result.IsSuccess && result.Value == 1) + // { + // Console.WriteLine("foo" + result.Value); + // } + + + } + + } diff --git a/CSharpFunctionalExtensions.Analyzers.Tests/CSharpFunctionalExtensions.Analyzers.Tests.csproj b/CSharpFunctionalExtensions.Analyzers.Tests/CSharpFunctionalExtensions.Analyzers.Tests.csproj index 8fcab80..dafd118 100644 --- a/CSharpFunctionalExtensions.Analyzers.Tests/CSharpFunctionalExtensions.Analyzers.Tests.csproj +++ b/CSharpFunctionalExtensions.Analyzers.Tests/CSharpFunctionalExtensions.Analyzers.Tests.csproj @@ -22,7 +22,7 @@ runtime; build; native; contentfiles; analyzers; buildtransitive all - + diff --git a/CSharpFunctionalExtensions.Analyzers.Tests/UseResultValueWithoutCheckTests/UseResultValueWithoutCheckTests.cs b/CSharpFunctionalExtensions.Analyzers.Tests/UseResultValueWithoutCheckTests/UseResultValueWithoutCheckTests.cs index e1b02d9..9c4b850 100644 --- a/CSharpFunctionalExtensions.Analyzers.Tests/UseResultValueWithoutCheckTests/UseResultValueWithoutCheckTests.cs +++ b/CSharpFunctionalExtensions.Analyzers.Tests/UseResultValueWithoutCheckTests/UseResultValueWithoutCheckTests.cs @@ -63,6 +63,7 @@ public async Task TestNoDiagnostic_AccesValueOnResultObject_WithCheckIsSuccess(s [InlineData("""if(result.IsFailure || result.Value == 1) Console.WriteLine("foo");""")] [InlineData("""if(!result.IsSuccess || result.Value > 1) Console.WriteLine("foo");""")] [InlineData("var x = !result.IsFailure ? result.Value: 0;")] + [InlineData("var x = result.IsSuccess ? Foo(result.Value): 0;")] public async Task TestNoDiagnostic_AccesValueOnResultObject_WithCheckIsFailure(string source) { await VerifyNoDiagnosticAsync(AddContext(source), options: CSharpTestOptions()); @@ -91,13 +92,44 @@ public async Task Test_AccessValueAfterCheckForFailure(string source) { await VerifyDiagnosticAsync(AddContext(source), options: CSharpTestOptions()); } + + [Theory] + [InlineData("if(!result.IsFailure) Console.WriteLine(result.Value);")] + [InlineData(" var x = result.IsFailure ? 0 : result.Value;")] + [InlineData(" var x = !result.IsSuccess ? 0 : result.Value;")] + [InlineData(" var x = result.IsSuccess ? result.Value : 0;")] + [InlineData(" var x = !result.IsFailure ? result.Value : 0;")] + public async Task TestNoDiagnostic_AccessValueAfterCheckForFailure(string source) + { + await VerifyNoDiagnosticAsync(AddContext(source), options: CSharpTestOptions()); + } + + [Theory] + [InlineData("return result.IsSuccess ? result.Value: 0;")] + [InlineData("return result.IsSuccess ? Foo(result.Value) : 0;")] + [InlineData("return !result.IsSuccess ? 0: result.Value;")] + [InlineData("return !result.IsSuccess ? 0: Foo(result.Value);")] + public async Task TestNoDiagnostic_AccessValueAfterCheckForSuccessWithReturn(string source) + { + await VerifyNoDiagnosticAsync(AddReturnValueContext(source), options: CSharpTestOptions()); + } + + [Theory] + [InlineData("return !result.IsSuccess ? [|result.Value|]: 0;")] + [InlineData("return !result.IsSuccess ? Foo([|result.Value|]) : 0;")] + [InlineData("return result.IsSuccess ? 0: [|result.Value|];")] + [InlineData("return result.IsSuccess ? 0: Foo([|result.Value|]);")] + public async Task TestDiagnostic_AccessValueAfterCheckForSuccessWithReturn(string source) + { + await VerifyDiagnosticAsync(AddReturnValueContext(source), options: CSharpTestOptions()); + } [Fact] public async Task Test_AccessWithinReturnStatement() { await VerifyDiagnosticAsync( $$""" - using System; + using System; using CSharpFunctionalExtensions; public class Class2 @@ -193,6 +225,30 @@ public void GetId(int a) var result = {{result}}; {{testString}} } + public int Foo(int bar) + { + return bar; + } + } + """; + + private string AddReturnValueContext(string testString, string result = "Result.Success(1)") => + $$""" + using System; + using CSharpFunctionalExtensions; + + public class FunctionsWithResultObject + { + public int GetId(int a) + { + var result = {{result}}; + {{testString}} + } + + public int Foo(int bar) + { + return bar; + } } """; } diff --git a/CSharpFunctionalExtensions.Analyzers/CSharpFunctionalExtensions.Analyzers.csproj b/CSharpFunctionalExtensions.Analyzers/CSharpFunctionalExtensions.Analyzers.csproj index 6fb8c46..0a87bb3 100644 --- a/CSharpFunctionalExtensions.Analyzers/CSharpFunctionalExtensions.Analyzers.csproj +++ b/CSharpFunctionalExtensions.Analyzers/CSharpFunctionalExtensions.Analyzers.csproj @@ -36,7 +36,7 @@ - + all runtime; build; native; contentfiles; analyzers; buildtransitive diff --git a/CSharpFunctionalExtensions.Analyzers/UseResultValueWithoutCheck/ResultValueWalker.cs b/CSharpFunctionalExtensions.Analyzers/UseResultValueWithoutCheck/ResultValueWalker.cs index b325862..531749e 100644 --- a/CSharpFunctionalExtensions.Analyzers/UseResultValueWithoutCheck/ResultValueWalker.cs +++ b/CSharpFunctionalExtensions.Analyzers/UseResultValueWithoutCheck/ResultValueWalker.cs @@ -73,14 +73,35 @@ private void NodeWalkerInternal(SyntaxNode node) private void CheckTernaryCondition(ConditionalExpressionSyntax ternary) { _result.CheckResult = DetermineCheckResult(ternary.Condition); + _result.Terminated = true; + bool whenTrueContainsAccess = ContainsMatchingMemberAccess(ternary.WhenTrue, _memberAccessValueResult); + bool whenFalseContainsAccess = ContainsMatchingMemberAccess(ternary.WhenFalse, _memberAccessValueResult); + _result.AccessedValue = _result.CheckResult switch { - CheckResult.CheckedSuccess => ternary.WhenTrue == _memberAccessValueResult && ternary.WhenFalse != _memberAccessValueResult, - CheckResult.CheckedFailure => ternary.WhenFalse == _memberAccessValueResult && ternary.WhenTrue != _memberAccessValueResult, + CheckResult.CheckedSuccess => whenTrueContainsAccess && !whenFalseContainsAccess, + CheckResult.CheckedFailure => whenTrueContainsAccess, _ => _result.AccessedValue }; } + + private bool ContainsMatchingMemberAccess(SyntaxNode expression, SyntaxNode targetMemberAccess) + { + if (expression.IsEquivalentTo(targetMemberAccess)) + return true; + + foreach (var child in expression.DescendantNodes().OfType()) + { + if (child is MemberAccessExpressionSyntax memberAccess && memberAccess.IsEquivalentTo(targetMemberAccess)) + return true; + + if (ContainsMatchingMemberAccess(child, targetMemberAccess)) + return true; + + } + return false; + } private CheckResult BinaryExpressionSyntax(BinaryExpressionSyntax binaryExpression) { switch (binaryExpression.OperatorToken.Kind()) @@ -178,20 +199,28 @@ private CheckResult DetermineCheckResult(ExpressionSyntax condition) case ConditionalExpressionSyntax ternary: return DetermineCheckResult(ternary.Condition); case IsPatternExpressionSyntax isPatternExpressionSyntax: + var info = DebugInfo(isPatternExpressionSyntax); return isPatternExpressionSyntax.Pattern switch { RecursivePatternSyntax recursivePatternSyntax => CheckedRecusivePattern(recursivePatternSyntax), - _ => throw new ArgumentOutOfRangeException() }; - - break; - case SwitchExpressionSyntax switchExpressionSyntax: - throw new NotImplementedException(); + // case SwitchExpressionSyntax switchExpressionSyntax: + // throw new NotImplementedException(); } return CheckResult.Unchecked; } + private static string DebugInfo(ExpressionSyntax syntax) + { + var syntaxTree = syntax.SyntaxTree; + var lineSpan = syntaxTree.GetLineSpan(syntax.Span); + var startLineNumber = lineSpan.StartLinePosition.Line; // 0-based + var startCharacterPosition = lineSpan.StartLinePosition.Character; // Ook 0-based + var filePath = syntaxTree.FilePath; + return $"File: {filePath} Line: {startLineNumber +1}, Char: {startCharacterPosition + 1}"; + } + private static bool IsSuccess(string leftExpression, string rightExpression) { return leftExpression.Contains("IsSuccess") && rightExpression == "true" diff --git a/README.md b/README.md index c92c035..4b613d2 100644 --- a/README.md +++ b/README.md @@ -77,4 +77,4 @@ Contributions are welcome! Please feel free to open an issue or submit a pull re ## License -MIT +MIT \ No newline at end of file diff --git a/build/Build.cs b/build/Build.cs index 8346761..8e7934a 100644 --- a/build/Build.cs +++ b/build/Build.cs @@ -55,7 +55,7 @@ class Build : NukeBuild static readonly string PackageContentType = "application/octet-stream"; static string ChangeLogFile => RootDirectory / "CHANGELOG.md"; - + string BranchSpec => GitHubActions?.Ref; bool IsTag => BranchSpec != null && BranchSpec.Contains("refs/tags", StringComparison.OrdinalIgnoreCase); @@ -73,7 +73,7 @@ class Build : NukeBuild }); Target Restore => _ => _ - .DependsOn(Clean) + .DependsOn(Clean) .Executes(() => { DotNetRestore(s => s.SetProjectFile(SourceDirectory)); @@ -130,7 +130,8 @@ class Build : NukeBuild Target PublishToGithub => _ => _ .Description($"Publishing to Github for Development only.") .Triggers(CreateRelease) - .OnlyWhenStatic(() => Repository.IsOnDevelopBranch() && GitHubActions is not null || GitHubActions?.IsPullRequest == true) + .OnlyWhenStatic(() => + (Repository.IsOnDevelopBranch() || Repository.IsOnReleaseBranch()) && GitHubActions is not null || GitHubActions?.IsPullRequest == true) .Executes(() => { ArtifactsDirectory.GlobFiles(ArtifactsType) @@ -149,8 +150,7 @@ class Build : NukeBuild .DependsOn(Pack) .Requires(() => NugetApiUrl) .Requires(() => NugetApiKey) - .OnlyWhenDynamic(() => IsTag, "No Tag added to commit") - .Triggers(CreateRelease) + .OnlyWhenDynamic(() => IsTag|| Repository.IsOnReleaseBranch(), "No Tag added to commit") .Executes(() => { DotNetNuGetPush(s => @@ -162,10 +162,11 @@ class Build : NukeBuild ) ); }); - + Target CreateRelease => _ => _ .Description($"Creating release for the publishable version.") - .OnlyWhenStatic(() => Repository.IsOnMainOrMasterBranch() || Repository.IsOnReleaseBranch()) + //Create release is not working just turn it off for now. + .OnlyWhenStatic(() => false && (Repository.IsOnMainOrMasterBranch() || Repository.IsOnReleaseBranch())) .Executes(async () => { var credentials = new Credentials(GitHubActions.Token); @@ -225,7 +226,7 @@ static async Task UploadReleaseAssetToGithub(Release release, string asset) Log.Information("Https URL = {Value}", Repository.HttpsUrl); Log.Information("SSH URL = {Value}", Repository.SshUrl); - + Log.Information("ChangeLogFile = {Value}", ChangeLogFile); Log.Information("SourceDirectory = {Value}", SourceDirectory); Log.Information("RootDirectory = {Value}", RootDirectory); diff --git a/changelog.md b/changelog.md index a05766a..795f99f 100644 --- a/changelog.md +++ b/changelog.md @@ -14,3 +14,5 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Enhanced Roslyn analyzer to support Switch Expression Syntax. The analyzer now correctly identifies and validates usage of `IsSuccess` and `Value` within C# switch expressions, providing a more comprehensive code quality check. +## 1.4.1 / 2024-03-22 +- Addressed multiple issues related to pattern matching and complex conditional logic that were impacting code analysis accuracy.