Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable Microsoft Code Analysis #1782

Open
wants to merge 28 commits into
base: development
Choose a base branch
from
Open

Enable Microsoft Code Analysis #1782

wants to merge 28 commits into from

Conversation

amoerie
Copy link
Collaborator

@amoerie amoerie commented May 2, 2024 •
edited

Fixes #1781

Checklist

  • The pull request branch is in sync with latest commit on the fo-dicom/development branch
  • I have updated API documentation
  • I have included unit tests
  • I have updated the change log
  • I am listed in the CONTRIBUTORS file

Changes proposed in this pull request:

  • Enable CodeAnalysis "Minimum" = the most basic set of checks that Microsoft deemed as essential. We can dial it up to "Recommended" later
  • I manually edited the .editorconfig to set the severity of several reliability and performance related checks to "Warning". For example, the "ConfigureAwait" check was not part of the "Minimum" set, so I had to enable this separately.
  • Fix various warnings that surfaced after turning on the analysis.

Screenshot of what it looks like now if you forget ConfigureAwait(false):

image

amoerie
amoerie commented May 2, 2024
View reviewed changes
FO-DICOM.Core/Network/PDU.cs
var length = (ushort) ms.Position;
rawPdu.GetCommonFields(buffer, length);
await stream.WriteAsync(buffer.Bytes, 0, RawPDU.CommonFieldsLength + length, cancellationToken).ConfigureAwait(false);
RawPDU pdu = null;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small note here: "await using" is compiler syntax sugar for DisposeAsync being called inside an automatically generated finally block.
Unfortunately there is no easy way to say "also add ConfigureAwait(false)", so this syntax sugar needed to be unwrapped manually.

I could also suppress the warning here, since the method RawPDU.DisposeAsync doesn't actually do anything async for now, but that felt more dangerous.

@amoerie amoerie mentioned this pull request May 6, 2024
Merged
5 tasks
amoerie added 26 commits May 23, 2024 08:49
@amoerie
Enable minimal code analysis
64cd8f4
@amoerie
CA1822 Make members static if they do not access instance data
43bc2eb
@amoerie
CA1834 Optimize StringBuilder usage
2b19147
@amoerie
CA1829 Use Length or Count when possible
77c67e9
@amoerie
CA1825 Avoid unnecessary zero-length array allocations
a0fa530
@amoerie
CA1822 Member can be marked static
28b81b5
@amoerie
CA1816 Call GC.SuppressFinalize(this) when disposing
20fd1e2
@amoerie
CA2208 Throw ArgumentXXXExceptions correctly
017ed7d
@amoerie
Add a few analyzer suppressions
9024a48
@amoerie
Don't let vulnerabilities emit compiler errors
a606806
@amoerie
CA1822 Make members static if they do not access instance data
abe8e3a
@amoerie
Remove duplicate package reference
1ecd278
@amoerie
CA1825 Avoid unnecessary zero-length array allocations
31323f1
@amoerie
CA1822 Member can be marked static
ded3a89
@amoerie
CA1816 Call GC.SuppressFinalize(this) when disposing
a973902
@amoerie
CA1829 Use Length or Count when possible
6f7d2bf
@amoerie
CA1834 Optimize StringBuilder usage
6c68f3d
@amoerie
Suppress CA2211 in unit tests
e220bba
@amoerie
Log messages should be compile time constants
d2a9e8c
@amoerie
Re-add missing ConfigureAwait that was removed for analyzer testing p…
8b70d73 
…urposes
@amoerie
Set analysis level to 8
582b48d
@amoerie
Explicitly set severity of reliability & performance related checks t…
1f3f397 
…o "Warning"
@amoerie
Add missing ConfigureAwait(false) and fix more compile time log messages
ce6ddcd
@amoerie
Unwrap "await using" so we can call ConfigureAwait when disposing
e0ac41b
@amoerie
Tidy up remaining few issues
04a7270
@amoerie
Tidy up remaining few issues
f0af8c3
@amoerie amoerie marked this pull request as ready for review May 23, 2024 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Microsoft Code Analysis
1 participant
@amoerie