Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix S1067 FP: Conditionals in pattern should not be considered as a unit for the whole pattern #6429

Closed
martin-strecker-sonarsource opened this issue Nov 23, 2022 · 5 comments · Fixed by #6500
Assignees
Labels
Area: C# C# rules related issues. Type: False Positive Rule IS triggered when it shouldn't be.
Milestone

Comments

@martin-strecker-sonarsource
Copy link
Contributor

martin-strecker-sonarsource commented Nov 23, 2022

Description

In pattern like this:

_ = new Exception() is ArgumentException // Noncompliant FP
{
    Message: "A" or "B" or "C",
    InnerException:
    {
        Message: "D" or "E" or "F",
    }
};

The or patterns are counted as a single group and S1067 is raised because more than 3 or are present. Because the two checks don't belong together directly the property patterns should be considered a "conditional root". Also, note that other sub-pattern kinds are also roots (declaration pattern, var pattern, list pattern, and maybe more).

Some test cases were added in #6428

C# 11 list pattern are also affected

_ = new[] { 1, 2, 3 } is [1 or 2 or 3, 1 or 2 or 3, _]; // Noncompliant FP
@martin-strecker-sonarsource martin-strecker-sonarsource added Type: False Positive Rule IS triggered when it shouldn't be. Area: C# C# rules related issues. Area: C# 11 labels Nov 23, 2022
@martin-strecker-sonarsource martin-strecker-sonarsource added this to the 8.50 milestone Nov 23, 2022
@csaba-sagi-sonarsource csaba-sagi-sonarsource modified the milestones: 8.50, 8.51 Dec 1, 2022
@martin-strecker-sonarsource martin-strecker-sonarsource changed the title FP S1067: Conditionals in pattern should not be considered as a unit for the whole pattern Fix S1067 FP: Conditionals in pattern should not be considered as a unit for the whole pattern Dec 8, 2022
@martin-strecker-sonarsource
Copy link
Contributor Author

Preliminary Peach validation until AX0U1INEAng_OcXOZhgF:

Lost: Tenary operator condition (Not exactly defined by the RSpec)

https://peach.sonarsource.com/code?id=dnn&selected=dnn%3ADNN+Platform%2FLibrary%2FEntities%2FUsers%2FUserController.cs&line=1674

if the conditions appear in the "Condition" of the ternary, then the ternary is ignored, because we do not step only into the WhenTrue and WhenFalse part but not the Condition part.

var userId = user != null && user.UserID > 0 && !user.IsDeleted && (showSuperUsers || !user.IsSuperUser) 
    ? user.UserID
    : 0;

More:
https://peach.sonarsource.com/code?id=dnn&selected=dnn%3ADNN+Platform%2FLibrary%2FServices%2FAuthentication%2FAuthenticationController.cs&line=259

https://peach.sonarsource.com/code?id=dnspy&selected=dnspy%3AdnSpy%2FdnSpy%2FText%2FFormatting%2FWpfTextViewLine.cs&line=651

https://peach.sonarsource.com/code?id=dotnet-runtime&selected=dotnet-runtime%3Asrc%2Flibraries%2FMicrosoft.Bcl.AsyncInterfaces%2Fsrc%2FSystem%2FThreading%2FTasks%2FSources%2FManualResetValueTaskSourceCore.cs&line=91

https://peach.sonarsource.com/code?id=dotnet-runtime&selected=dotnet-runtime%3Asrc%2Flibraries%2FSystem.ComponentModel.TypeConverter%2Fsrc%2FMS%2FInternal%2FXml%2FLinq%2FComponentModel%2FXComponentModel.cs&line=311

https://peach.sonarsource.com/code?id=dotnet-runtime&selected=dotnet-runtime%3Asrc%2Flibraries%2FSystem.Data.Common%2Fsrc%2FSystem%2FData%2FDataRowCollection.cs&line=102

https://peach.sonarsource.com/code?id=dotnet-runtime&selected=dotnet-runtime%3Asrc%2Flibraries%2FSystem.Data.Common%2Fsrc%2FSystem%2FData%2FSelect.cs&line=695

https://peach.sonarsource.com/code?id=dotnet-runtime&selected=dotnet-runtime%3Asrc%2Flibraries%2FSystem.Linq.Expressions%2Fsrc%2FSystem%2FLinq%2FExpressions%2FCompiler%2FILGen.cs&line=802

Lost: XOR (Not specified by the RSpec)

https://peach.sonarsource.com/code?id=dnspy&selected=dnspy%3AdnSpy%2FdnSpy.Contracts.Debugger%2FBreakpoints%2FModules%2FDbgModuleBreakpointSettings.cs&line=99

https://peach.sonarsource.com/code?id=dnspy&selected=dnspy%3AdnSpy%2FdnSpy.Contracts.DnSpy%2FHex%2FEditor%2FHexGroups%2FLocalGroupOptions.cs&line=159

https://peach.sonarsource.com/code?id=dotnet-runtime&selected=dotnet-runtime%3Asrc%2Flibraries%2FSystem.Net.Http%2Fsrc%2FSystem%2FNet%2FHttp%2FHeaders%2FCacheControlHeaderValue.cs&line=333

Lost: Nested in other binary expression

https://peach.sonarsource.com/code?id=dnspy&selected=dnspy%3AExtensions%2FILSpy.Decompiler%2FICSharpCode.Decompiler%2FICSharpCode.Decompiler%2FILAst%2FInitializerPeepholeTransforms.cs&line=341

Ternary is on the right-hand side of a != expression. We should step into all kinds of binary expressions but not count them as complexity-increasing.

recheck =
    inlining.numLdloc.GetOrDefault(v) != 1 ||
    inlining.numLdloca.GetOrDefault(v) != totalElementCount + (expr.Code == ILCode.Call ? 1 : 0)) ||
    (inlining.numStloc.GetOrDefault(v) != (expr.Code == ILCode.Call ? 0 : 1));

More
https://peach.sonarsource.com/code?id=dnspy&selected=dnspy%3ALibraries%2FICSharpCode.TreeView%2FFlatListTreeNode.cs&line=58

https://peach.sonarsource.com/code?id=dotnet-runtime&selected=dotnet-runtime%3Asrc%2Flibraries%2FMicrosoft.CSharp%2Fsrc%2FMicrosoft%2FCSharp%2FRuntimeBinder%2FSemantics%2FMemberLookup.cs&line=176

& and |

https://peach.sonarsource.com/code?id=dotnet-runtime&selected=dotnet-runtime%3Asrc%2Flibraries%2FMicrosoft.CSharp%2Fsrc%2FMicrosoft%2FCSharp%2FRuntimeBinder%2FSymbolTable.cs&line=117

https://peach.sonarsource.com/code?id=dotnet-runtime&selected=dotnet-runtime%3Asrc%2Flibraries%2FSystem.Composition.TypedParts%2Fsrc%2FSystem%2FComposition%2FTypedParts%2FActivationFeatures%2FOnImportsSatisfiedFeature.cs&line=42

https://peach.sonarsource.com/code?id=dotnet-runtime&selected=dotnet-runtime%3Asrc%2Flibraries%2FSystem.Configuration.ConfigurationManager%2Fsrc%2FSystem%2FConfiguration%2FConfigurationElement.cs&line=645

Lost: In invocation arguments (TP)

We need to consider some expressions as "begins a new tree". Invocation arguments belong here.

https://peach.sonarsource.com/code?id=dotnet-runtime&selected=dotnet-runtime%3Asrc%2Flibraries%2FSystem.Data.Common%2Fsrc%2FSystem%2FData%2FSQLTypes%2FSQLBoolean.cs&line=280

https://peach.sonarsource.com/code?id=dotnet-runtime&selected=dotnet-runtime%3Asrc%2Flibraries%2FSystem.Data.Common%2Fsrc%2FSystem%2FData%2FSQLTypes%2FSQLDateTime.cs&line=522

@andrei-epure-sonarsource
Copy link
Contributor

do you plan to make a fix for these @martin-strecker-sonarsource (as it was a side-effect of the refactoring)?

@martin-strecker-sonarsource
Copy link
Contributor Author

For the first one we need to do something (the condition part of an outermost ternary is not checked at all at the moment). For the others, I would define other binaries as "transparent" (except maybe XOR, | and &). Invocation arguments start a new root IMO on the other hand (TP).

@andrei-epure-sonarsource
Copy link
Contributor

I agree that invocation arguments start a new root.

@martin-strecker-sonarsource
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: C# C# rules related issues. Type: False Positive Rule IS triggered when it shouldn't be.
Projects
None yet
4 participants