-
Notifications
You must be signed in to change notification settings - Fork 1.1k
chore: clean up build warnings #7522
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.
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
chore: clean up build warnings #7522
Conversation
…ess xUnit1030 warnings
…ss xUnit1030 warnings
removed tons of useless XML-DOCs too
Fix CS0659 and CS0420 warnings in core Akka libraries This commit addresses two types of build warnings: 1. CS0659: Missing GetHashCode implementation - Added GetHashCode implementations to classes in EventSequences.cs - Converted test classes in AtLeastOnceDeliverySpec.cs to C# records 2. CS0420: Volatile field access warning - Removed redundant 'volatile' keyword from _statusDotNotCallMeDirectly field in Mailbox.cs - Kept explicit Volatile.Read/Write method calls for proper memory barriers These changes maintain the original behavior while eliminating the warnings, improving code quality and maintainability.
This commit addresses the CS8632 warnings that occur when nullable reference type annotations (like string?) are used in files with #nullable enable directives but in projects where nullable reference types are not enabled at the project level. Changes: 1. Added CS8632 to the NoWarn property in src/Directory.Build.props to suppress these warnings during build 2. Created a GlobalSuppressions.cs file with a SuppressMessage attribute for CS8632 with a clear justification This approach preserves the intent of the code by keeping the #nullable enable directives and nullable annotations intact, while eliminating the build warnings. It also maintains forward compatibility if the project later decides to fully enable nullable reference types across all projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Detailed my changes
| and make everything fully async in the future | ||
| --> | ||
| <NoWarn>$(NoWarn);CS1591;xUnit1013;xUnit1031</NoWarn> | ||
| <NoWarn>$(NoWarn);CS1591;xUnit1013;xUnit1031;CS8632</NoWarn> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disables https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/nullable-warnings#configure-nullable-context - which is basically a warning that we have some non-nullable code calling code with nullable attributes. We're naturally going to encounter that since we're gradually moving code over to using nullable - so this cuts down on a fairly large and growing number of warnings.
| using System.Diagnostics.CodeAnalysis; | ||
|
|
||
| // Suppress CS8632 warnings about nullable reference type annotations | ||
| [assembly: SuppressMessage("Style", "CS8632:The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.", Justification = "Files with #nullable enable directives use nullable annotations correctly")] No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public void Fail(string format = "", params object[] args) | ||
| { | ||
| Assert.True(false, string.Format(format, args)); | ||
| Assert.Fail(string.Format(format, args)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleans up an xUnit warning
| { | ||
| public LeaseTimeoutException(string message) { } | ||
| public LeaseTimeoutException(string message, System.Exception innerEx) { } | ||
| protected LeaseTimeoutException(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No inheritors for this class, so this eliminates that warning
| using Xunit; | ||
| using Xunit.Abstractions; | ||
| using FluentAssertions; | ||
| #pragma warning disable CS0659 // Type overrides Object.Equals(object o) but does not override Object.GetHashCode() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got rid of the warning and fixed the problems instead. These are all related to the test messages themselves so no real impact on Akka.NET.
| if (numberOfChildren > 20) return numberOfChildren + " children"; | ||
| var sb = new StringBuilder(); | ||
|
|
||
| sb.Append("Children:\n ").AppendJoin("\n ", InternalChildren, ChildStatsAppender); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So something that is pretty funny: we've had our own AppendJoin function for years, going back to 2014. At some point in the future .NET added one, officially, to StingBuilder and now our extension method conflicts with this - resulting in us ToString()-ing a delegate rather than actually emitting the data we expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ChildStatsAppender was the delegate getting stringified
| if (numberOfChildren > 10) | ||
| sb.Append(numberOfChildren).Append(" children\n"); | ||
| else | ||
| sb.Append("Children:\n ").AppendJoin("\n ", InternalChildren, ChildStatsAppender).Append('\n'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the same stringification of delegate warning
| */ | ||
| private volatile ActorCell _actor = null; | ||
| private volatile SystemMessage _systemQueueDoNotCallMeDirectly = null; // null by default | ||
| private volatile int _statusDotNotCallMeDirectly; //0 by default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either use the volatile keyword or use Volatile.Read / Volatile.Write consistently, but don't do both.
| /// </summary> | ||
| [Serializable] | ||
| public sealed class Reset | ||
| public new sealed class Reset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Poor naming choices here, but hiding the APIs doesn't hurt anything in our tests or the accessibility of the underlying class. This is pure compiler ceremony.
|
|
||
| public override bool Equals(object obj) => Equals(obj as RouterConfig); | ||
|
|
||
| public override int GetHashCode() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed to override GetHashCode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Current
|
Changes
c;leaning up various build warnings
Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):