Analyzer suggestion: Track allocations in implicit conversion operators

dotnet / roslyn-analyzers

MIT License

1.58k stars 463 forks source link

Analyzer suggestion: Track allocations in implicit conversion operators #4200

Open Mrnikbobjeff opened 4 years ago

Mrnikbobjeff commented 4 years ago

The guidelines for implicit conversion operators state that a user defined implicit conversion operator should not allocate on the heap. An analyzer could be used to detect cases where the conversion operator returns a reference type, suggesting the change to explicit to not conflict with the guideline.

Joe4evr commented 4 years ago

This should be in dotnet/roslyn-analyzers.

KalleOlaviNiemitalo commented 4 years ago

The implicit conversion from string to System.Xml.Linq.XName would trigger that analyzer. The conversion returns a reference type but caches the instance so that later conversions with the same input need not allocate. I suppose this would have to be documented as a valid reason to suppress the warning.

https://github.com/dotnet/runtime/blob/38017c3935de95d0335bac04f4901ddfc2718656/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XName.cs#L99-L109

mjsabby commented 4 years ago

At least some such allocations are already in this analyzer, https://github.com/microsoft/RoslynClrHeapAllocationAnalyzer and probably others in roslyn-analyzers repo.

ericstj commented 4 years ago

This should be in dotnet/roslyn-analyzers.

@jeffhandley where would you suggest we track new analyzer suggestions?

stephentoub commented 4 years ago

where would you suggest we track new analyzer suggestions?

Strawman:

Either

Everything in roslyn-analyzers since that's where the work will actually be done.

If it's about language (e.g. a rule flagging that a new language construct can be used, or a rule flagging that some language pattern allocates, or the like), it should be in roslyn or roslyn-analyzers.
If it's about framework types, it should be in runtime, since our goal is that new analyzers come part and parcel with new APIs.
If it's about framework design guidelines, it should be in runtime, since this repo is the heart of .NET and guidance on the shape of APIs moving forward.

I think we're currently on the latter path.

ericstj commented 4 years ago

@mavasani or @stephentoub can you help transfer this, looks like I don't have write permissions.

mavasani commented 4 years ago

@genlu @sharwell seems related to performance sensitive analyzers?