sonar-dotnet Implement S2225 - ToString should not return null

Fixes #6112

Jun 14 '22 12:06 Corniel

I did a rebase to resolve the syntax facade conflicts. You can pull that change.

Sep 13 '22 14:09 pavel-mikula-sonarsource

I also detect ToString() => SomeCondtion ? null : "" now.

Sep 14 '22 06:09 Corniel

@Corniel Looks good so far, there are 3 unresolved conversations left to address.

I've pushed the rspec scaffolding.

Sep 19 '22 12:09 pavel-mikula-sonarsource

/azp run Sonar.Net

Sep 20 '22 06:09 pavel-mikula-sonarsource

Azure Pipelines successfully started running 1 pipeline(s).

Sep 20 '22 06:09 azure-pipelines[bot]

ITs failed on

vbc : error AD0001: Analyzer 'SonarAnalyzer.Rules.VisualBasic.ToStringShouldNotReturnNull' threw an exception of type 'System.InvalidOperationException' with message 'NodeExpression can not be performed on a IdentifierNameSyntax node.'. [C:\ProgramData\vsts-agent\1\s\analyzers\its\sources\Ember-MM\EmberAPI\EmberAPI.vbproj]
vbc : error AD0001: Analyzer 'SonarAnalyzer.Rules.VisualBasic.ToStringShouldNotReturnNull' threw an exception of type 'System.InvalidOperationException' with message 'NodeExpression can not be performed on a MemberAccessExpressionSyntax node.'. [C:\ProgramData\vsts-agent\1\s\analyzers\its\sources\Ember-MM\EmberAPI\EmberAPI.vbproj]
vbc : error AD0001: Analyzer 'SonarAnalyzer.Rules.VisualBasic.ToStringShouldNotReturnNull' threw an exception of type 'System.InvalidOperationException' with message 'NodeExpression can not be performed on a IdentifierNameSyntax node.'. [C:\ProgramData\vsts-agent\1\s\analyzers\its\sources\Ember-MM\EmberAPI\EmberAPI.vbproj]
vbc : error AD0001: Analyzer 'SonarAnalyzer.Rules.VisualBasic.ToStringShouldNotReturnNull' threw an exception of type 'System.InvalidOperationException' with message 'NodeExpression can not be performed on a TernaryConditionalExpressionSyntax node.'. [C:\ProgramData\vsts-agent\1\s\analyzers\its\sources\Ember-MM\EmberAPI\EmberAPI.vbproj]

Sep 20 '22 06:09 pavel-mikula-sonarsource

To reproduce the IT, you should be able to:

rebuild the solution
run this from analyzers\its directory

.\regression-test.ps1 -ruleId S2225 -project Ember-MM

The full build log will be in analyzers\its\output\

Sep 20 '22 06:09 pavel-mikula-sonarsource

I'm facing a fundamental problem:

    private bool ReturnsNull(SyntaxNode node) =>
        Language.Syntax.IsNullLiteral(node)
        || Conditionals(node).Any(ReturnsNull)
        || ReturnsNull(Language.Syntax.NodeExpression(node));

Would solve all my problems if Language.Syntax.NodeExpression would not throw on nodes that do not contain an exception. But it does. And In VB. the conditional does no contain expressions per se (where C# does).

Sep 20 '22 09:09 Corniel

Will changing the logic help? Basically to let Conditionals be recursive, and deal with all levels of nesting of conditionals. And ReturnsNull be nonrecursive?

Sep 27 '22 14:09 pavel-mikula-sonarsource

Will changing the logic help? Basically to let Conditionals be recursive, and deal with all levels of nesting of conditionals. And ReturnsNull be nonrecursive?

If you do the recursion per implementation it is not that hard. It is just feels wrong. The SyntaxFacade is powerful, but at this point not fully consistent. Some methods return null on this kind of mappings, and some throw. That was the point I was trying to make.

Sep 27 '22 19:09 Corniel

Let's move the recursion into a language-specific part. It's just way easier.

The facade methods are supposed to be called only for well-~~known~~expected types. Otherwise adding more items later could bite in the old usages. Some of them are just proxies to existing methods, and those behave differently.

Sep 28 '22 07:09 pavel-mikula-sonarsource

Actually, the recursion logic could be on the common side too. Just calling the abstract part more times.

Sep 28 '22 07:09 pavel-mikula-sonarsource

Actually, the recursion logic could be on the common side too. Just calling the abstract part more times.

The point is that the whenTrue and whenFalse in c# are ParenthesizedExpressionSyntax's that contain an epression themselves, but not when the represent a literal. It's working now.

Sep 30 '22 10:09 Corniel

@pavel-mikula-sonarsource Can you have a look?

Oct 07 '22 08:10 Corniel

/azp run Sonar.Net.External

Oct 11 '22 14:10 pavel-mikula-sonarsource

Azure Pipelines successfully started running 1 pipeline(s).

Oct 11 '22 14:10 azure-pipelines[bot]

I need to test the external pipeline, so I'll play with that here a bit

Oct 11 '22 14:10 pavel-mikula-sonarsource

/azp run Sonar.Net

Oct 13 '22 16:10 pavel-mikula-sonarsource

Azure Pipelines successfully started running 1 pipeline(s).