Create rule S3063: Add vbnet language #1524

github-actions · 2023-01-30T10:19:26Z

You can preview this rule here (updated a few minutes after each push).

sonarsource-next · 2023-02-02T16:13:24Z

SonarQube Quality Gate for 'rspec-tools'

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarsource-next · 2023-02-02T16:14:59Z

SonarQube Quality Gate for 'rspec-frontend'

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

martin-strecker-sonarsource · 2023-02-14T13:32:38Z

rules/S3063/csharp/rule.adoc

@@ -1,16 +1,77 @@
-include::../rule.adoc[]
+``++StringBuilder++`` instances that are never ``++ToString++``ed needlessly clutter the code, and worse are a drag on performance. Either they should be removed, or the missing ``++ToString++`` call added.


Suggested change

``++StringBuilder++`` instances that are never ``++ToString++``ed needlessly clutter the code, and worse are a drag on performance. Either they should be removed, or the missing ``++ToString++`` call added.

`StringBuilder` instances that are never `ToString`ed needlessly clutter the code, and worse are a drag on performance. Either they should be removed, or the missing `ToString` call added.

martin-strecker-sonarsource · 2023-02-14T13:32:54Z

rules/S3063/csharp/rule.adoc

-(visible only on this page)
+[source,csharp]
+----
+public void doSomething(List<string> strings) {


Suggested change

public void doSomething(List<string> strings) {

public void DoSomething(List<string> strings) {

martin-strecker-sonarsource · 2023-02-14T13:33:13Z

rules/S3063/csharp/rule.adoc

-(visible only on this page)
+[source,csharp]
+----
+public void doSomething(List<string> strings) {


Suggested change

public void doSomething(List<string> strings) {

public void DoSomething(List<string> strings) {

martin-strecker-sonarsource · 2023-02-14T13:33:31Z

rules/S3063/csharp/rule.adoc

+or
+[source,csharp]
+----
+public void doSomething(List<string> strings) {


Suggested change

public void doSomething(List<string> strings) {

public void DoSomething(List<string> strings) {

martin-strecker-sonarsource · 2023-02-14T13:33:56Z

rules/S3063/csharp/rule.adoc

+[source,csharp]
+----
+public void doSomething(List<string> strings) {
+  StringBuilder sb = new StringBuilder();  // Noncompliant


Suggested change

StringBuilder sb = new StringBuilder(); // Noncompliant

StringBuilder sb = new StringBuilder();

martin-strecker-sonarsource · 2023-02-14T14:41:21Z

rules/S3063/vbnet/rule.adoc

@@ -0,0 +1,75 @@
+``++StringBuilder++`` instances that are never ``++ToString++``ed needlessly clutter the code, and worse are a drag on performance. Either they should be removed, or the missing ``++ToString++`` call added.


Suggested change

``++StringBuilder++`` instances that are never ``++ToString++``ed needlessly clutter the code, and worse are a drag on performance. Either they should be removed, or the missing ``++ToString++`` call added.

`StringBuilder` instances that are never `ToString`ed needlessly clutter the code, and worse are a drag on performance. Either they should be removed, or the missing `ToString` call added.

martin-strecker-sonarsource · 2023-02-14T14:42:15Z

rules/S3063/vbnet/rule.adoc

+
+[source,vbnet]
+----
+Public Sub doSomething(ByVal strings As List(Of String))


Suggested change

Public Sub doSomething(ByVal strings As List(Of String))

Public Sub DoSomething(ByVal strings As List(Of String))

martin-strecker-sonarsource · 2023-02-14T14:42:32Z

rules/S3063/vbnet/rule.adoc

+
+[source,vbnet]
+----
+Public Sub doSomething(ByVal strings As List(Of String))


Suggested change

Public Sub doSomething(ByVal strings As List(Of String))

Public Sub DoSomething(ByVal strings As List(Of String))

martin-strecker-sonarsource · 2023-02-14T14:42:39Z

rules/S3063/vbnet/rule.adoc

+or
+[source,vbnet]
+----
+Public Sub doSomething(ByVal strings As List(Of String))


Suggested change

Public Sub doSomething(ByVal strings As List(Of String))

Public Sub DoSomething(ByVal strings As List(Of String))

martin-strecker-sonarsource · 2023-02-14T14:43:06Z

rules/S3063/vbnet/rule.adoc

+No issue is reported when ``++StringBuilder++`` is:
+
+* passed as method arguments, on the grounds that it will likely ``++ToString++``ed there.


Suggested change

No issue is reported when ``++StringBuilder++`` is:

* passed as method arguments, on the grounds that it will likely ``++ToString++``ed there.

No issue is reported when `StringBuilder` is:

* passed as method arguments, on the grounds that it will likely `ToString`ed there.

martin-strecker-sonarsource · 2023-02-14T17:52:00Z

rules/S3063/csharp/rule.adoc

@@ -1,16 +1,58 @@
-include::../rule.adoc[]
+`StringBuilder` instances that are never `ToString`ed needlessly clutter the code, and worse are a drag on performance. Either they should be removed, or the missing `ToString` call added.


https://discuss.sonarsource.com/t/csharpsquid-s3063-description-seems-weird/13189 is complaining about

ToStringed

being odd and I agree. Can you find a better wording?

martin-strecker-sonarsource · 2023-02-14T17:54:29Z

rules/S3063/csharp/rule.adoc

+var sb = new StringBuilder(); // Compliant
+sb.GetChunks();
+----
+* passed as method argument, on the grounds that it will likely `ToString`ed there.


ToStringed

martin-strecker-sonarsource · 2023-02-14T17:55:41Z

rules/S3063/csharp/rule.adoc

+
+No issue is reported when `StringBuilder` is:
+
+* accessed through a ``CopyTo()`` or ``GetChunks()`` invocation.


Suggested change

* accessed through a ``CopyTo()`` or ``GetChunks()`` invocation.

* accessed through a `CopyTo()` or `GetChunks()` invocation.

martin-strecker-sonarsource · 2023-02-14T17:57:21Z

rules/S3063/csharp/rule.adoc

+[source,csharp]
+----
+var sb = new StringBuilder(); // Compliant
+sb.GetChunks();


I don't think we need this example. The description is self-explanatory.

martin-strecker-sonarsource · 2023-02-14T17:59:08Z

rules/S3063/csharp/rule.adoc

+    sb.Append(str).Append(", ");
+    // ...
+  }
+  _logger.LogInformation(sb.toString, DateTimeOffset.UtcNow);


The ILogger.LogInformation doesn't require a DateTimeOffset.UtcNow to be passed.

Suggested change

_logger.LogInformation(sb.toString, DateTimeOffset.UtcNow);

_logger.LogInformation(sb.toString);

martin-strecker-sonarsource · 2023-02-14T18:00:18Z

rules/S3063/csharp/rule.adoc

+* retrieved by a custom function
+[source,csharp]
+----
+var sb = GetStringBuilder(); // Compliant
+----


Suggested change

* retrieved by a custom function

[source,csharp]

----

var sb = GetStringBuilder(); // Compliant

----

* retrieved by a custom function (`var sb = GetStringBuilder();`)

martin-strecker-sonarsource · 2023-02-14T18:04:57Z

rules/S3063/csharp/rule.adoc

+
+No issue is reported when `StringBuilder` is:
+
+* accessed through a ``CopyTo()`` or ``GetChunks()`` invocation.


All items in a list should start with an uppercase and end with a dot (see e.g. https://github.com/SonarSource/rspec/blob/1b5fe36f3f98b48a9db7966ff6b6402d3db02242/rules/S6472/implementation.adoc)

Suggested change

* accessed through a ``CopyTo()`` or ``GetChunks()`` invocation.

* Accessed through a ``CopyTo()`` or ``GetChunks()`` invocation.

martin-strecker-sonarsource · 2023-02-14T18:07:55Z

rules/S3063/vbnet/rule.adoc

+
+No issue is reported when `StringBuilder` is:
+
+* accessed through a ``CopyTo()`` or ``GetChunks()`` invocation.


Same as for C#

martin-strecker-sonarsource · 2023-02-15T09:56:17Z

rules/S3063/csharp/rule.adoc

@@ -1,16 +1,49 @@
-include::../rule.adoc[]
+`StringBuilder` instances that are not accessed through a `ToString()` invocation needlessly clutter the code, and worse are a drag on performance. Either they should be removed, or the missing `ToString()` call should be added.


Maybe like this?

Suggested change

`StringBuilder` instances that are not accessed through a `ToString()` invocation needlessly clutter the code, and worse are a drag on performance. Either they should be removed, or the missing `ToString()` call should be added.

`StringBuilder` instances that never build a `string` clutter the code and worse are a drag on performance. Either they should be removed, or the missing `ToString()` call should be added.

Yes that's better, thanks

martin-strecker-sonarsource · 2023-02-15T09:59:06Z

rules/S3063/csharp/rule.adoc

+    sb.Append(str).Append(", ");
+    // ...
+  }
+  _logger.LogInformation(sb.toString);


I think the _ is a violation of one of our rules.

Suggested change

_logger.LogInformation(sb.toString);

logger.LogInformation(sb.ToString());

martin-strecker-sonarsource · 2023-02-15T10:00:42Z

rules/S3063/csharp/rule.adoc

+
+* Accessed through a `CopyTo()` or `GetChunks()` invocation.
+* Passed as method argument, on the grounds that it will likely accessed through a `ToString()` invocation there.
+* A parameter of the current method.


Suggested change

* A parameter of the current method.

* Passed in as a parameter to the current method, on the grounds that the callee will materialize the string.

martin-strecker-sonarsource · 2023-02-15T10:01:08Z

rules/S3063/csharp/rule.adoc

+No issue is reported when `StringBuilder` is:
+
+* Accessed through a `CopyTo()` or `GetChunks()` invocation.
+* Passed as method argument, on the grounds that it will likely accessed through a `ToString()` invocation there.


Suggested change

* Passed as method argument, on the grounds that it will likely accessed through a `ToString()` invocation there.

* Passed as a method argument, on the grounds that it will likely be accessed through a `ToString()` invocation there.

martin-strecker-sonarsource · 2023-02-15T10:02:38Z

rules/S3063/csharp/rule.adoc

+
+No issue is reported when `StringBuilder` is:
+
+* Accessed through a `CopyTo()` or `GetChunks()` invocation.


Should we add the Length access and the indexer access?

I don't know; I was wondering the same. I decided to skip them to simplify the exception paragraph since both are not that common, but I might be wrong.

Maybe like this?

Suggested change

* Accessed through a `CopyTo()` or `GetChunks()` invocation.

* Accessed through `sb.CopyTo()`, `sb.GetChunks()`, `sb.Length`, or `sb[index]`.

Since we get rid of the exception examples it's not that complicated anymore so maybe we can add them for clarity.

Oh I only just saw your comment. I created a sub-list for them, but your suggestion may be better indeed, let me change that real quick.

martin-strecker-sonarsource

LGTM. Some typos to fix left.

martin-strecker-sonarsource · 2023-02-15T10:50:53Z

rules/S3063/vbnet/rule.adoc

+        sb.Append(str).Append(", ")
+    Next
+
+    My.Application.Log.WriteEntry(sb.toString())


Suggested change

My.Application.Log.WriteEntry(sb.toString())

My.Application.Log.WriteEntry(sb.ToString())

martin-strecker-sonarsource · 2023-02-15T10:51:27Z

rules/S3063/vbnet/rule.adoc

+No issue is reported when `StringBuilder` is:
+
+* Accessed through `sb.CopyTo()`, `sb.GetChunks()`, `sb.Length`, or `sb(index)`.
+* Passed as a method arguments, on the grounds that it will likely be accessed through a `ToString()` invocation there.


Suggested change

* Passed as a method arguments, on the grounds that it will likely be accessed through a `ToString()` invocation there.

* Passed as a method argument, on the grounds that it will likely be accessed through a `ToString()` invocation there.

sonarsource-next · 2023-02-15T11:01:09Z

SonarQube Quality Gate for 'rspec-tools'

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarsource-next · 2023-02-15T11:01:46Z

SonarQube Quality Gate for 'rspec-frontend'

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

github-actions bot assigned cristian-ambrosini-sonarsource Jan 30, 2023

github-actions bot added the dotnet label Jan 30, 2023

cristian-ambrosini-sonarsource changed the title ~~Create rule S3063~~ Create rule S3063: Add vbnet language Jan 30, 2023

cristian-ambrosini-sonarsource requested a review from martin-strecker-sonarsource January 30, 2023 10:43

cristian-ambrosini-sonarsource mentioned this pull request Feb 2, 2023

New rule S3063: "StringBuilder" data should be used SonarSource/sonar-dotnet#6659

Closed

cristian-ambrosini-sonarsource marked this pull request as ready for review February 2, 2023 16:11

cristian-ambrosini-sonarsource and others added 4 commits February 14, 2023 15:43

Add vbnet to rule S3063

e1e1be2

Add adoc for CS/VB

a0461f4

Add exceptions

9de270d

Add invocations exceptions

cc40ef6

cristian-ambrosini-sonarsource force-pushed the rule/S3063-add-vbnet branch from f03ca8b to cc40ef6 Compare February 14, 2023 14:45

martin-strecker-sonarsource requested changes Feb 14, 2023

View reviewed changes

Addressed PR comments

e8a60e4

cristian-ambrosini-sonarsource requested a review from martin-strecker-sonarsource February 14, 2023 15:26

martin-strecker-sonarsource requested changes Feb 14, 2023

View reviewed changes

CristianAmbrosini added 2 commits February 15, 2023 09:10

Semplifications, better wording on the descriptions

6d2843b

Small description fix

be8b428

cristian-ambrosini-sonarsource requested a review from martin-strecker-sonarsource February 15, 2023 08:13

martin-strecker-sonarsource requested changes Feb 15, 2023

View reviewed changes

Add indexer and .Length expression exceptions and fix description

d8fde13

cristian-ambrosini-sonarsource requested a review from martin-strecker-sonarsource February 15, 2023 10:32

sub-list to inline

2d36200

martin-strecker-sonarsource approved these changes Feb 15, 2023

View reviewed changes

Fix typos

5c2f745

cristian-ambrosini-sonarsource merged commit a8b5a83 into master Feb 15, 2023

cristian-ambrosini-sonarsource deleted the rule/S3063-add-vbnet branch February 15, 2023 12:07

		@@ -1,16 +1,77 @@
		include::../rule.adoc[]
		``++StringBuilder++`` instances that are never ``++ToString++``ed needlessly clutter the code, and worse are a drag on performance. Either they should be removed, or the missing ``++ToString++`` call added.

	``++StringBuilder++`` instances that are never ``++ToString++``ed needlessly clutter the code, and worse are a drag on performance. Either they should be removed, or the missing ``++ToString++`` call added.
	`StringBuilder` instances that are never `ToString`ed needlessly clutter the code, and worse are a drag on performance. Either they should be removed, or the missing `ToString` call added.

	public void doSomething(List<string> strings) {
	public void DoSomething(List<string> strings) {

	StringBuilder sb = new StringBuilder(); // Noncompliant
	StringBuilder sb = new StringBuilder();

		@@ -0,0 +1,75 @@
		``++StringBuilder++`` instances that are never ``++ToString++``ed needlessly clutter the code, and worse are a drag on performance. Either they should be removed, or the missing ``++ToString++`` call added.

	Public Sub doSomething(ByVal strings As List(Of String))
	Public Sub DoSomething(ByVal strings As List(Of String))

		No issue is reported when ``++StringBuilder++`` is:

		* passed as method arguments, on the grounds that it will likely ``++ToString++``ed there.

		@@ -1,16 +1,58 @@
		include::../rule.adoc[]
		`StringBuilder` instances that are never `ToString`ed needlessly clutter the code, and worse are a drag on performance. Either they should be removed, or the missing `ToString` call added.


		No issue is reported when `StringBuilder` is:

		* accessed through a ``CopyTo()`` or ``GetChunks()`` invocation.

	* accessed through a ``CopyTo()`` or ``GetChunks()`` invocation.
	* accessed through a `CopyTo()` or `GetChunks()` invocation.

	_logger.LogInformation(sb.toString, DateTimeOffset.UtcNow);
	_logger.LogInformation(sb.toString);

		@@ -1,16 +1,49 @@
		include::../rule.adoc[]
		`StringBuilder` instances that are not accessed through a `ToString()` invocation needlessly clutter the code, and worse are a drag on performance. Either they should be removed, or the missing `ToString()` call should be added.

	`StringBuilder` instances that are not accessed through a `ToString()` invocation needlessly clutter the code, and worse are a drag on performance. Either they should be removed, or the missing `ToString()` call should be added.
	`StringBuilder` instances that never build a `string` clutter the code and worse are a drag on performance. Either they should be removed, or the missing `ToString()` call should be added.

	_logger.LogInformation(sb.toString);
	logger.LogInformation(sb.ToString());

	* A parameter of the current method.
	* Passed in as a parameter to the current method, on the grounds that the callee will materialize the string.

	* Passed as method argument, on the grounds that it will likely accessed through a `ToString()` invocation there.
	* Passed as a method argument, on the grounds that it will likely be accessed through a `ToString()` invocation there.

	* Accessed through a `CopyTo()` or `GetChunks()` invocation.
	* Accessed through `sb.CopyTo()`, `sb.GetChunks()`, `sb.Length`, or `sb[index]`.

	My.Application.Log.WriteEntry(sb.toString())
	My.Application.Log.WriteEntry(sb.ToString())

	* Passed as a method arguments, on the grounds that it will likely be accessed through a `ToString()` invocation there.
	* Passed as a method argument, on the grounds that it will likely be accessed through a `ToString()` invocation there.

Create rule S3063: Add vbnet language #1524

Create rule S3063: Add vbnet language #1524

Conversation

github-actions bot commented Jan 30, 2023

sonarsource-next bot commented Feb 2, 2023

sonarsource-next bot commented Feb 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristian-ambrosini-sonarsource Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

martin-strecker-sonarsource left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarsource-next bot commented Feb 15, 2023

sonarsource-next bot commented Feb 15, 2023

cristian-ambrosini-sonarsource Feb 15, 2023 •

edited

Loading