search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.25k stars 4.73k forks source link

Port remaining FxCop rules into .NET as Roslyn Analyzers #61964

Open Evangelink opened 2 years ago

Evangelink commented 2 years ago

In roslyn-analyzers there is a bunch of tickets open related to porting some old FxCop rules. Because most of these are dated from 2015, we were wondering if it would be worth having them under review as their relevance or the context in which they apply may have changed.

I have tried to filter down the ones related to .NET API but I may have missed or included incorrect ones (if you want to review them all, please look at https://github.com/dotnet/roslyn-analyzers/issues?q=is%3Aopen+label%3AFxCop-Port+-label%3A%22Needs-Fixer%22):

(tagging @mavasani )

Native resources/Interop

WinForms

Serialization

Others

dotnet-issue-labeler[bot] commented 2 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

mavasani commented 2 years ago

@jmarolf @mikadumont

stephentoub commented 2 years ago

We should just close any of these that are no longer relevant / stale / etc.

@AaronRobinsonMSFT / @jkoritzinsky / @elinor-fung to comment on the interop rules.

I suggest we close all of the serialization ones. We're in the process of deprecating BinaryFormatter, we're no longer encouraging implementation of ISerializable nor implementing it ourselves on new types, etc. @GrabYourPitchforks

@merriemcgaw for WinForms.

Evangelink commented 2 years ago

@stephentoub Out of curiosity, are you simply no longer encouraging the implementation of ISerializable or shall we go even one step further and have analyzers guarding against (there could be a dotnet version check)?

merriemcgaw commented 2 years ago

@RussKie can you look at the WinForms ones and see if they make any sense in the current context?

GrabYourPitchforks commented 2 years ago

@Evangelink Basically, we're no longer marking any new types as [Serializable] or applying the ISerializable interface. There are some exceptions for the sake of compatibility, but those use cases are definitely in the minority.

RussKie commented 2 years ago

WinForms:

These look worthy of a port.

Over the months we had several team discussions and bounced ideas about how we can enhance the developer and end user experiences with various Windows Forms analyzers. I also had an offline thread with @jmarolf @sharwell @mavasani about the same subject, and for the general purpose analyzers (like these) https://github.com/dotnet/roslyn-analyzers looked like a good home.

AaronRobinsonMSFT commented 2 years ago

The Interop team has gone through the list. The below list contains what we think makes sense to port. All others can be closed.

https://github.com/dotnet/roslyn-analyzers/issues/420 https://github.com/dotnet/roslyn-analyzers/issues/421 https://github.com/dotnet/roslyn-analyzers/issues/425 https://github.com/dotnet/roslyn-analyzers/issues/428 https://github.com/dotnet/roslyn-analyzers/issues/430

Evangelink commented 2 years ago

@AaronRobinsonMSFT Would it be possible to get a short description on why the team thinks the following two rules are not worth being implemented? https://github.com/dotnet/roslyn-analyzers/issues/395 https://github.com/dotnet/roslyn-analyzers/issues/530

Evangelink commented 2 years ago

@stephentoub @GrabYourPitchforks Could you please confirm that's ok to close all the tickets about serialization?

The tickets in the Other section are still waiting for review/decision.

AaronRobinsonMSFT commented 2 years ago

@Evangelink Sure. Would you like the comments here or in the specific issues?

Evangelink commented 2 years ago

Maybe directly on the tickets but I am fine here too. The two rules seem to make sense to me (like if you own native or disposable resources you must be disposable, and for the 2nd what's the point of using an interop if the fwk allows for the same fnctionality).

AaronRobinsonMSFT commented 2 years ago

@Evangelink I've commented on each issue directly.

stephentoub commented 2 years ago

@stephentoub @GrabYourPitchforks Could you please confirm that's ok to close all the tickets about serialization?

Yes, go ahead. Thanks.

jeffhandley commented 2 years ago

@buyaa-n & @carlossanlop Please review the analyzers in the 'Other' category to determine if we want to bring those for API Review and include them in our plans. Thanks!

jeffhandley commented 2 years ago

that's ok to close all the tickets about serialization

Done

jeffhandley commented 2 years ago

We were not able to finish this list. Changing the milestone to Future to keep the epic alive. More of these analyzers will be considered as part of .NET 8 planning.