Skip to content

Conversation

@elachlan
Copy link
Contributor

@elachlan elachlan commented Jan 8, 2022

@elachlan
Copy link
Contributor Author

elachlan commented Jan 8, 2022

Error:
.packages/microsoft.codeanalysis.collections/4.0.0-4.21379.20/contentFiles/cs/netstandard2.0/Internal/ThrowHelper.cs(274,28): error SA1122: (NETCORE_ENGINEERING_TELEMETRY=Build) Use string.Empty for empty strings

The issue is in microsoft.codeanalysis.collections. I am unsure how to fix it, other than fixing it upstream.

@elachlan
Copy link
Contributor Author

elachlan commented Jan 9, 2022

@sharwell, apparently "" is already interned by the compiler. Was the idea of SA1122 to reduce string allocations? or is the hardcoded string used for another purpose?

Based on my reading, the advantage is in readability and to reduce the chance of a hidden zero width space character.

There was acutally one disadvantage that I saw:
"Another difference is that String.Empty generates larger CIL code"
https://stackoverflow.com/a/17386465/908889

It also isn't a constant:
https://stackoverflow.com/questions/507923/why-isnt-string-empty-a-constant

Which results in some usages being incorrect:
https://stackoverflow.com/a/13703103/908889

runtime also turned down adding a constant and suggested using "":
dotnet/runtime#60508

@elachlan
Copy link
Contributor Author

elachlan commented Jan 9, 2022

@Forgind You may want to review this with the team? We can set the analyzer to none if it isn't helpful.

@elachlan
Copy link
Contributor Author

@rainersigwald Should we just disable this rule?

@sharwell
Copy link
Contributor

IMO this rule is more tedious than it's worth.

@elachlan
Copy link
Contributor Author

If the team gives the okay, I will mark the severity for this analyzer as none.

@danmoseley
Copy link
Member

In dotnet/runtime we prefer "" . I think string.Empty is a useless vestige.

@Forgind Forgind merged commit ebab967 into dotnet:main Jan 12, 2022
@sharwell sharwell changed the title SA1122 Use string.Empty for empty strings Disable SA1122 (Use string.Empty for empty strings) Jan 12, 2022
@elachlan elachlan deleted the SA1122 branch January 12, 2022 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants