Skip to content

Commit

Permalink
Disable 'use pattern matching' when eliding multiple calls that pass …
Browse files Browse the repository at this point in the history
…by ref (#76183)
  • Loading branch information
CyrusNajmabadi authored Dec 3, 2024
2 parents bd11ad7 + df45e32 commit 4de50b6
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 19 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,19 @@ private Binary(AnalyzedPattern leftPattern, AnalyzedPattern rightPattern, bool i
if (!AreEquivalent(target.Syntax, compareTarget.Syntax))
return null;

// An &&/|| expression that looks the same on both sides. This looks like something we could merge into an
// 'and/or' pattern that only executes the target once, and compares the result to multiple constants.
// However, we disqualify certain forms as they strongly imply that executing the target multiple times
// is necessary. For example:
//
// `if (ReadValue(x, ref index) == A && ReadValue(x, ref index) == B)`
//
// This should not get converted to `if (ReadValue(x, ref index) == A and B)`.
//
// This list is not exhaustive and can be expanded as needed.
if (target.Syntax.DescendantNodesAndSelf().OfType<ArgumentSyntax>().Any(a => a.RefKindKeyword.Kind() is SyntaxKind.RefKeyword))
return null;

return new Binary(leftPattern, rightPattern, isDisjunctive, token, target);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,20 @@
using Microsoft.CodeAnalysis.CSharp.Extensions;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;
using Microsoft.CodeAnalysis.Shared.Extensions;

namespace Microsoft.CodeAnalysis.CSharp.UsePatternCombinators;

using static AnalyzedPattern;

[DiagnosticAnalyzer(LanguageNames.CSharp)]
internal sealed class CSharpUsePatternCombinatorsDiagnosticAnalyzer :
AbstractBuiltInCodeStyleDiagnosticAnalyzer
internal sealed class CSharpUsePatternCombinatorsDiagnosticAnalyzer() :
AbstractBuiltInCodeStyleDiagnosticAnalyzer(
IDEDiagnosticIds.UsePatternCombinatorsDiagnosticId,
EnforceOnBuildValues.UsePatternCombinators,
CSharpCodeStyleOptions.PreferPatternMatching,
s_safePatternTitle,
s_safePatternTitle)
{
private const string SafeKey = "safe";

Expand All @@ -32,15 +36,6 @@ internal sealed class CSharpUsePatternCombinatorsDiagnosticAnalyzer :
hasAnyCodeStyleOption: true,
s_unsafePatternTitle);

public CSharpUsePatternCombinatorsDiagnosticAnalyzer()
: base(IDEDiagnosticIds.UsePatternCombinatorsDiagnosticId,
EnforceOnBuildValues.UsePatternCombinators,
CSharpCodeStyleOptions.PreferPatternMatching,
s_safePatternTitle,
s_safePatternTitle)
{
}

public static bool IsSafe(Diagnostic diagnostic)
=> diagnostic.Properties.ContainsKey(SafeKey);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,16 @@
namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.UsePatternCombinators;

[Trait(Traits.Feature, Traits.Features.CodeActionsUsePatternCombinators)]
public class CSharpUsePatternCombinatorsDiagnosticAnalyzerTests : AbstractCSharpDiagnosticProviderBasedUserDiagnosticTest_NoEditor
public sealed class CSharpUsePatternCombinatorsDiagnosticAnalyzerTests(ITestOutputHelper logger)
: AbstractCSharpDiagnosticProviderBasedUserDiagnosticTest_NoEditor(logger)
{
private static readonly ParseOptions CSharp9 = TestOptions.RegularPreview.WithLanguageVersion(LanguageVersion.CSharp9);

private static readonly OptionsCollection s_disabled = new OptionsCollection(LanguageNames.CSharp)
private static readonly OptionsCollection s_disabled = new(LanguageNames.CSharp)
{
{ CSharpCodeStyleOptions.PreferPatternMatching, new CodeStyleOption2<bool>(false, NotificationOption2.None) }
};

public CSharpUsePatternCombinatorsDiagnosticAnalyzerTests(ITestOutputHelper logger)
: base(logger)
{
}

internal override (DiagnosticAnalyzer, CodeFixProvider) CreateDiagnosticProviderAndFixer(Workspace workspace)
=> (new CSharpUsePatternCombinatorsDiagnosticAnalyzer(), new CSharpUsePatternCombinatorsCodeFixProvider());

Expand Down Expand Up @@ -694,4 +690,34 @@ private void M(object o)
}
""");
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/75122")]
public async Task TestNotWithMultipleCallsToInvocationWithRefArgument()
{
await TestMissingAsync(
"""
using System;
static class DataUtils
{
internal static string ReadLine(byte[] bytes, ref int index)
{
throw new NotImplementedException();
}
}
class C
{
public void Main(byte[] bytes)
{
int index = 0;
if ([|DataUtils.ReadLine(bytes, ref index) != "YAFC" || DataUtils.ReadLine(bytes, ref index) != "ProjectPage"|])
{
throw new InvalidDataException();
}
}
}
""");
}
}

0 comments on commit 4de50b6

Please sign in to comment.