.NET 8 developers can verify more APIs for correct usage to speed up their development

@CollinAlpert

With the Roslyn Analyzers we built in .NET 5, 6 and 7, we've found that developers benefit from analyzers that verify correct usage of .NET Libraries APIs. We plan to put more effort on adding valuable analyzers in .NET 8, we will continue working with the community to foster contributions into our collection of analyzers and code fixers.

Community contributors: If you'd like to work on one of the issues marked as "up for grabs", please add a comment directly on that issue asking to get it assigned to you. Please check the getting started guide for reference about how to start and what's need to be done.

Analyzers Planned for .NET 8

Approved analyzers

No	Status	Issue	Estimate
1	✔️ by @CollinAlpert	Prefer Dictionary<K, V>.TryAddValue(key) over guarded Add(key)	Medium
2	✔️ by @CollinAlpert	Prefer .Length/Count/IsEmpty over Any()	Medium
3	✔️ by @fowl2	Do not use OfType() with impossible types	Small
4	✔️ by @steveberdy	Extract array of const to static readonly field	Medium
5	in PR by @psxvoid	Constructor parameters should match property names	Medium
6	in PR by @NewellClark	Prevent wrong usage of string Split	Medium
7	in PR by @NewellClark	Prefer static ReadOnlySpan properties over static readonly byte[] fields	Medium
8	in PR by @NewellClark	Do not pass Utf8JsonReader by value	Medium
9	✔️ @geeknoid	Recommend use of concrete types to maximize devirtualization potential	Medium
10	✔️ @stephentoub	Convert argument null checks to ArgumentNullException.ThrowIfNull	Medium
11	✔️ @Youssef1313	Using StartsWith instead of IndexOf == 0	Medium
12	@NewellClark	Avoid implicitly allocating params arrays in loops	Medium
13	✔️ @carlossanlop	Recommend string.ToLower{Invariant}().Contains/IndexOf/StartsWith("lowercasestring") be replaced by case-insensitive comparison	Medium
14	up for grabs	Usage of ToString in logging source generator method call sites	Medium
15	✔️ @carlossanlop	string.ToLower() == otherString.ToLower()	Small
16	✔️ @mrahhal	Recommend string.StartsWith('c') instead of string.StartsWith("c")	Medium
17	up for grabs	Replacing direct LoggerExtensions.Log* usage with source-generated logging methods	Medium
18	up for grabs	Prefer ValueTuple over Tuple	Medium
19	✔️ @mpidash	Report inefficient use of sets	Small
20	up for grabs	Introduce an analyzer to recommend the more efficient string splitting features in .NET 8	Medium
21	partially implemented @stephentoub	Recommend upgrading to CompositeFormat for enhanced performance	Large
22	up for grabs	Prefer Dictionary.Remove(key, out value) over Dictionary.this[key], followed by Dictionary.Remove(key)	Medium
23	up for grabs	Hide Thread.VolatileRead and Thread.VolatileWrite	Medium
24	in PR by @mpidash	Simplify calls to string.Substring and Memory/Span.Slice	Small
25	@mpidash	Detect the problem of using a System.Type overload instead of the generic overload	Medium
26	up for grabs	Treat calls to ROS op_Equality as warning	Small
27	up for grabs	Detect non-cancelable Task.Delay passed to Task.WhenAny	Medium
28	@tannergooding	Upgrade from platform specific to cross platform intrinsics	Small
29	@tannergooding	Suggest more optimal SIMD patterns where applicable	Medium
30	up for grabs	Adjust/simplify code for numeric IntPtr	Medium
31	up for grabs	Analyzer/fixer for converting Stream.Read calls to ReadAtLeast and ReadExactly	Medium
32	up for grabs	Exceptions thrown inside async methods should be wrapped by Task.FromException	Medium
33	@CollinAlpert	Make types declared in an executable internal	Small
34	up for grabs	Unawaited / returned tasks created with a disposable in using block for that disposable	Large
35	@pedrodeandrade	Async methods that don't take a CancellationToken	Small
36	up for grabs	Use IndexOfAnyValues instead of inlined or cached array	Small
37	✔️ @jozkee	Add an analyzer that warns on single-use JsonSerializerOptions instances

Other analyzer proposals that need to be prepared for API review

CC @stephentoub @jeffhandley @ericstj @carlossanlop

The .Any() => .Length != 0 warning makes no sense whatsoever.

.Any() should check under the hood whether the incoming IEnumerable is in fact an array, in which case it should do .Length != 0 automatically. LINQ methods already do this sort of optimization-based-on-the-type extensively, as far as I know.

We shouldn't be telling users to change their code in these cases, .Any() is without doubt clearer and more readable than .Length != 0. This was such a strange decision.

.Any() should check under the hood whether the incoming IEnumerable is in fact an array, in which case it should do .Length != 0 automatically. LINQ methods already do this sort of optimization-based-on-the-type extensively, as far as I know.

It does, and it's still more costly than the caller just doing it itself.

It does

Good to know.

and it's still more costly than the caller just doing it itself.

Obviously, but then again if Any() merits an analyzer/warning, so does every other LINQ method that does type-checking under the hood — in cases where the caller knows the specific type of the collection, no? Why make an exception for .Any() in this regard?
If you're in such a hot path that even the performance overhead of .Any() makes a difference, you shouldn't be using any LINQ anyway. But in most cases, the code clarity that LINQ methods offer (including .Any()) trumps the performance cost, that's why they exist in the first place. I don't understand why .Any() is being singled out here.

I don't understand why .Any() is being singled out here.

Likely because it's the most common usage in linq.

But I agree that, at least, this probably should have a lower diagnostic level than Suggestion by default. Most code won't benefit from this particular optimization, any code that is sensitive enough to benefit would probably be already aware of this. I also feel that Any() has become more readable overtime than Length/Count == 0. Maybe because it's a common interface that doesn't need to change when you're switching impls from arrays to collections and vice versa as you're developing. Having largely irrelevant squiggles randomly popping up isn't pretty, I always end up silencing this particular family of diags.

Just found this (via Nick Chapsas).

Re. #78606 - it's a pity that the word "ordinal" does not appear even once... especially wrt ignore-case. I think it should have featured in the discussion.

This table looks wrong to me. In every case, the assumption is that the string is linguistic, or is "linguistically relevant".

I refer you to:- https://learn.microsoft.com/en-us/dotnet/standard/base-types/best-practices-strings

Specifially I refer you to: https://learn.microsoft.com/en-us/dotnet/standard/base-types/best-practices-strings#choosing-a-stringcomparison-member-for-your-method-call

For over 20 years, my case-insensitve string comparisons have been wrt

Case-insensitive internal identifiers.
Case-insensitive identifiers in standards such as XML and HTTP.
File paths.
Registry keys and values.
Environment variables.
Resource identifiers (for example, handle names).
Case-insensitive security-related settings.

ergo... StringComparison.OrdinalIgnoreCase.

The number of times I have had to worry about case-insensitivity in linguistic data that's input by the user or displayed to the user: zero.

The more common case (by far) for case-insensitive string comparisons is for non-linguistic strings. And getting it right for non-linguistic strings is important for security.

In every single case in the table, I'd go for OrdinalIgnoreCase as the default analyzer recommendation, with the caveat that "if the string is linguistic, go for culture-ignore-case".

Instead of sending warning when using .Any() in arrays and list, why not to add the following bunch of code to the .Net sources:

    public static bool Any<T>(this IList<T> source) =>
        source.Count != 0;
    public static bool Any<T>(this T[] source) =>
        source.Length != 0;
    public static bool Any<T>(this ICollection source) =>
        source.Count != 0;

As .NET 8 development is wrapping up closing this issue. Thank you, all the contributors, for the hard work and hope you will continue contributing into new analyzers in .NET 9 as we will continue the effort in .NET 9, analyzers that haven't completed with this issue will be moved to a new issue for .NET 9.

@buyaa-n

@buyaa-n

I am concerned that analyzers #78606 and dotnet/roslyn-analyzers#6720 and possibly others that are related have been implemented incorrectly, and I left multiple comments 3 weeks ago explaining why - all of which have been ignored.

I would like to see some comment on these concerns. We don't want to wrap things up in an incorrect state.

My concerns are expressed here:-

.NET 8 developers can verify more APIs for correct usage to speed up their development #78442 (comment)
Recommend using Equals with StringComparison instead of string.ToLower() == otherString.ToLower() roslyn-analyzers#6720 (comment)
CA1862 suggestion changing semantics of the comparison. #89740 (comment)
Recommend using Equals with StringComparison instead of string.ToLower() == otherString.ToLower() roslyn-analyzers#6720 (comment)
Recommend using Equals with StringComparison instead of string.ToLower() == otherString.ToLower() roslyn-analyzers#6720 (comment)

@Bellarmine-Head

@Bellarmine-Head I assume your comments related to this issue which is not closed and still have 8.0 milestone. I expect @carlossanlop would take care of it within 8.0 timeframe.

@buyaa-n

@buyaa-n I added my comments to #89740 because you asked me to. I'm not sure what the issue is all about tbh (!) nor how it relates to my main concerns over issues:-

... which are closed/merged, but I believe have been implemented incorrectly for the reasons that I have stated multiple times.

It would be great if @carlossanlop and other experts could comment on these concerns.

@buyaa-n

@buyaa-n

To expand on my concerns...

Let us take the example of #78606 which was opened by @stephentoub

It's fixed by dotnet/roslyn-analyzers#6662

Nowhere in the discussion in those two pages is the word "ordinal". This was the first red flag for me.

Now, to take a small example, it seems to me that the analyzer will recommend that where you previously did:-

str.ToLowerInvariant().IndexOf(substr)

you should now do:-

str.IndexOf(substr, StringComparison.InvariantCultureIgnoreCase)

This seems reasonable. What you had before, you have afterwards, except arguably in a better form.

But you have overlooked something. This section in this long-standing (and correct) article, which has been around in one form or another since 2005, is very clear on the matter. For comparing:-

Case-insensitive internal identifiers.
Case-insensitive identifiers in standards such as XML and HTTP.
File paths.
Registry keys and values.
Environment variables.
Resource identifiers (for example, handle names).
Case-insensitive security-related settings.

you should be using StringComparison.OrdinalIgnoreCase.

For me this boils down to:-

"Case-insensitive string comparisons should always use StringComparison.OrdinalIgnoreCase except in the very rare cases where the strings you are comparing are linguistic in some shape or form."

Examples of linguistic strings: a blog piece the user has typed in; a date-time formatted in the user's culture for end-user display.

Why you would ever want to compare linguistic strings case-insensitively, I don't know. In 20 years, I've never done it. Comparing (non-linguistic) identifiers, on the other hand, is something you do all day, every day.

If our analyzer (and related ones; see links in my post above) doesn't even consider or mention or recommend the StringComparison.OrdinalIgnoreCase option, then in my view it has to be deemed as grossly deficient and represents three problems:-

it could cause Microsoft's own programmers to do the wrong thing when comparing identifiers
it can be argued that it's recommending non-Microsoft programmers to do the wrong thing
it's going against https://learn.microsoft.com/en-us/dotnet/standard/base-types/best-practices-strings#choosing-a-stringcomparison-member-for-your-method-call

Now, all of this is my opinion. The lack of response to these concerns, which I have written about in sundry places (see links above) concerns me.

Talk of wrapping things up for .NET 8 without addressing this issue concerns me.

I'm putting in a lot of effort here in an attempt to help... and I'm not getting much/any response from it.

@buyaa-n

@buyaa-n

To expand on my concerns...

Let us take the example of #78606 which was opened by @stephentoub

It's fixed by dotnet/roslyn-analyzers#6662

Nowhere in the discussion in those two pages is the word "ordinal". This was the first red flag for me.

Now, to take a small example, it seems to me that the analyzer will recommend that where you previously did:-
str.ToLowerInvariant().IndexOf(substr)
you should now do:-
str.IndexOf(substr, StringComparison.InvariantCultureIgnoreCase)
This seems reasonable. What you had before, you have afterwards, except arguably in a better form.

But you have overlooked something. This section in this long-standing (and correct) article, which has been around in one form or another since 2005, is very clear on the matter. For comparing:-

Case-insensitive internal identifiers.

Case-insensitive identifiers in standards such as XML and HTTP.

File paths.

Registry keys and values.

Environment variables.

Resource identifiers (for example, handle names).

Case-insensitive security-related settings.

you should be using StringComparison.OrdinalIgnoreCase.

For me this boils down to:-

"Case-insensitive string comparisons should always use StringComparison.OrdinalIgnoreCase except in the very rare cases where the strings you are comparing are linguistic in some shape or form."

Examples of linguistic strings: a blog piece the user has typed in; a date-time formatted in the user's culture for end-user display.

Why you would ever want to compare linguistic strings case-insensitively, I don't know. In 20 years, I've never done it. Comparing (non-linguistic) identifiers, on the other hand, is something you do all day, every day.

If our analyzer (and related ones; see links in my post above) doesn't even consider or mention or recommend the StringComparison.OrdinalIgnoreCase option, then in my view it has to be deemed as grossly deficient and represents three problems:-

it could cause Microsoft's own programmers to do the wrong thing when comparing identifiers

it can be argued that it's recommending non-Microsoft programmers to do the wrong thing

it's going against https://learn.microsoft.com/en-us/dotnet/standard/base-types/best-practices-strings#choosing-a-stringcomparison-member-for-your-method-call

Now, all of this is my opinion. The lack of response to these concerns, which I have written about in sundry places (see links above) concerns me.

Talk of wrapping things up for .NET 8 without addressing this issue concerns me.

I'm putting in a lot of effort here in an attempt to help... and I'm not getting much/any response from it.

The issue would be that changing stuff to Ordinal in such case would be a visible behavioural change, which is something that analyzers shouldn't always promote.

@MichalPetryka

@MichalPetryka

The issue would be that changing stuff to Ordinal in such case would be a visible behavioural change, which is something that analyzers shouldn't always promote.

I suspected that something of this sort was involved in the reasoning behind the analyzers, and I get your point.

The thing is, if you write code like:-

    if (stringOne.ToUpperInvariant() == "SOME_CONSTANT")

or

    if (stringOne.ToUpperInvariant() == stringTwo.ToUpperInvariant())

then you don't have any kind of OrdinalIgnoreCase option or opportunity (as far as I'm aware). It's always a cultural/linguistic comparison, even if it does use the InvariantCulture.

We've all written the code above (in some form or another) a thousand times, and it's been fine!!

But converting the code to the better String.Equals and StringComparison.XYZ alternative, as the analyzers correctly promote, gives programmers the much-needed opportunity to think about the context... are the strings cultural/linguistic in any way? or are they identifiers? If identifiers, then actually a much better way if we need to be case-insensitive (and the recommended Microsoft way for security and correct operation generally, since 2005) is to use OrdinalIgnoreCase.

For the analyzers to not mention OrdinalIgnoreCase seems to me to occlude this option, and seems to not urge programmers to think about the context. Which I think would be a big shame... an opportunity will have been missed. I worry that we will start to see a proliferation of instances of InvariantCultureIgnoreCase in codebases when OrdinalIgnoreCase is so obviously the correct choice.

I can tell you that in 20 years I have never had cause to use StringComparison options:-

CurrentCulture
CurrentCultureIgnoreCase
InvariantCulture
InvariantCultureIgnoreCase

As I said above, when on earth would you knowingly want to compare cultural / linguistically-relevant strings? Probably never, even though (oh the irony!) you probably did it all the time with:-

    if (stringOne.ToUpperInvariant() == stringTwo.ToUpperInvariant())

and everything was fine.

Bottom line: when comparing two strings case-insensitively, OrdinalIgnoreCase is always the correct choice, except for those ultra-rare occasions when you really do want to compare two linguistic strings in this way, even if you used to do if (stringOne.ToUpperInvariant() == stringTwo.ToUpperInvariant()) in the bad old days.

The analyzers should at least clue developers into the OrdinalIgnoreCase option, and in what contexts it should be used.

"Hey programmer...

You're using:-

if (stringOne.ToUpperInvariant() == stringTwo.ToUpperInvariant())

to do a case-insensitive comparison of two strings, which is fine up to a point. But we'd like you use:-

if (stringOne.Equals(stringTwo, StringComparison.InvariantCultureIgnoreCase))

instead, because [insert reasons].

This is better [see reasons], and is the same as what you were doing before. So it's a safe change to make. But wait: if your strings are identifiers (including file paths, Registry keys, case-insensitive internal identifiers, case-insensitive identifiers in standards such as XML and HTTP and case-insensitive security-related settings, etc. see here for the complete list then an even better option is to use:-

if (stringOne.Equals(stringTwo, StringComparison.OrdinalIgnoreCase))

but doing so will require you to test your code carefully in case the behaviour has changed.

For new code, it's best to use the StringComparison.OrdinalIgnoreCase option for comparing case-insensitive identifiers from the outset.

Thanks."

For case-sensitive string comparisons, we do

if (str1 == str2)

and this gives an ordinal comparison (i.e. StringComparison.Ordinal).

So these analyzers are concerned with case-insensitive comparisons for the most part.

Recommending to use String.Equals instead of if (stringOne.ToUpperInvariant() == stringTwo.ToUpperInvariant()) for performance reasons, while retaining maximum backwards compatibility (no change in behaviour) via use of StringComparison.InvariantCultureIgnoreCase is a worthy thing.

But a higher, better, nobler and more worthy thing is to recommend the use of StringComparison.OrdinalIgnoreCase for correct operation and good security - in a word: "safety".

Safety of implementation beats out perf any day of the week.

The downside is that ppl have to check that there is no change in behaviour, I get that.

Navigation Menu

Repository navigation

.NET 8 developers can verify more APIs for correct usage to speed up their development #78442

Analyzers Planned for .NET 8

Approved analyzers

Other analyzer proposals that need to be prepared for API review

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

.NET 8 developers can verify more APIs for correct usage to speed up their development #78442

Description

Analyzers Planned for .NET 8

Approved analyzers

Other analyzer proposals that need to be prepared for API review

Activity

aradalvand commented on Jul 20, 2023

stephentoub commented on Jul 20, 2023

aradalvand commented on Jul 20, 2023

mrahhal commented on Jul 20, 2023

Bellarmine-Head commented on Jul 22, 2023

mirmostafa commented on Jul 28, 2023

buyaa-n commented on Aug 14, 2023

Bellarmine-Head commented on Aug 15, 2023

buyaa-n commented on Aug 15, 2023

Bellarmine-Head commented on Aug 16, 2023

Bellarmine-Head commented on Aug 17, 2023

MichalPetryka commented on Aug 17, 2023

Bellarmine-Head commented on Aug 17, 2023

Bellarmine-Head commented on Aug 17, 2023

Bellarmine-Head commented on Aug 18, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions