dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.04k stars 4.03k forks source link

C#: Error check/scan/rename inactive #if blocks #37171

Open vsfeedback opened 5 years ago

vsfeedback commented 5 years ago

I realize there are going to be issues with this, but it isn't impossible.

A typical developer will spend most of the time editing with DEBUG enabled. Particularly if the target platform/environment is using the native compiler which requires a pretty long build time.

The result is "#if RELEASE" blocks failing to undergo the real-time scanning the rest of the code receives. They also fail to participate in symbol renaming, and thus renaming can blindly produce errors.

This does lead to the issue of when does a developer want a conditional block to be considered "currently dead code" fully ignored versus it being "live" code simply dependent upon a build option. There is also the issue of how #if blocks interact in combination.

A solution would seem to be supporting multiple secondary conditional compilation symbol lists with each list being a "live" build configuration, but the secondary lists wouldn't be used for the actual build. There would be a "primary" list used for the actual build.

The most critical part of this is symbol renaming. There currently is no reasonable way to have a solution-wide symbol rename catch both DEBUG and RELEASE (or other) #if blocks.

The situation can even produce a runtime error not caught by the compiler.

The given symbol X might be renamed to Z, Y renamed to X, and Z renamed to Y (or a new X introduced after the old X was renamed to Y) -- either intentionally or just as things evolve during development. The currently active DEBUG/RELEASE blocks will track the change correctly and report any issues. However the inactive RELEASE/DEBUG blocks will be blind to the change. Once later made active by changing the build type, there will be no error reported. The previously inactive blocks will find the symbol they were referencing, but not detect it's the wrong symbol now provided the type is compatible.

This issue has been moved from https://developercommunity.visualstudio.com/content/idea/627570/c-error-checkscanrename-inactive-if-blocks.html VSTS ticketId: 939897 These are the original issue comments:

Jane Wu [MSFT] on 7/1/2019, 04:14 AM (10 days ago):

Thank you for taking the time to provide your suggestion. We will do some preliminary checks to make sure we can proceed further. We’ll provide an update once the issue has been triaged by the product team.

Mika Dumont [MSFT] on 7/11/2019, 10:45 AM (6 hours ago):

Thank you for filing this feedback! I'm going to move this to our open source repository on GitHub for further discussion with the community.

jbennink commented 1 month ago

Came to this item from a closed/duplicate notification on the Developer Community.

I had opend an issue regarding something similar, but I noticed this issue while running code cleanup, ie. removing unsued usings. Since the Conditional blocks that are not active at the moment you run the cleanup are completely ignored Code cleanup removes usings for code that will be needed in RELEASE of in my case !DEBUG.

My item: https://developercommunity.visualstudio.com/t/Refactoring-should-include-all-IF-ENDI/10705014

I also found another item, evne older that was closed because...no activity: https://developercommunity.visualstudio.com/t/code-cleanup-removes-using-statements-fo/529034

And as this issue mentions refactoring is also very dangerous when using these conditionals.

CyrusNajmabadi commented 1 month ago

@jbennink you can place your usings inside ifdef blocks as well.