Annotate System.Diagnostics.EventLog with Nullable Reference Types

[RenderMichael]

Contributes to #90400

[Copilot]

Pull request overview

This PR enables Nullable Reference Types (NRT) annotations for the System.Diagnostics.EventLog library, contributing to issue #90400. The changes ensure that the codebase correctly expresses nullability intent through type annotations, improving type safety and reducing potential null reference exceptions.

Changes:

• Added nullable annotations (?) to parameters, return types, fields, and properties across the library where null values are possible
• Added [NotNullWhen] attributes to improve flow analysis for null checks
• Added [MemberNotNull] and [MemberNotNullWhen] attributes to help the compiler understand null-state after certain operations
• Removed <Nullable>disable</Nullable> from project files to enable nullable reference type checking
• Updated struct initialization to use constructor patterns instead of object initializers where appropriate

Reviewed changes

Copilot reviewed 37 out of 37 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
System.Diagnostics.EventLog.csproj	Enabled nullable reference types by removing disable directive
Messages/System.Diagnostics.EventLog.Messages.csproj	Removed nullable disable directive
ref/System.Diagnostics.EventLog.csproj	Removed nullable disable directive from reference assembly
ref/System.Diagnostics.EventLog.cs	Updated API signatures with nullable annotations
UnsafeNativeMethods.cs	Added nullable annotations to P/Invoke signatures and struct fields
EventLog.cs, EventLogInternal.cs	Added nullable annotations to public and internal APIs
EventLogEntry.cs, EventLogRecord.cs	Added nullable annotations to entry/record properties
EventLogReader.cs, EventLogWatcher.cs	Added nullable annotations to reader/watcher APIs
Various Reader classes	Added nullable annotations throughout event reader infrastructure
NetFrameworkUtils.cs	Added nullable annotations to utility methods
Interop files	Added nullable annotations to interop declarations

[krwq]

The error seem like they are already reported in those two issues:

• System.Security.Cryptography.CryptographicException : m_safeCertContext is an invalid handle. #124279
• Microsoft.Extensions.Hosting.Tests.ConsoleLifetimeExitTests.EnsureEnvironmentExitDoesntHang fails on .NET Framework #119545

[krwq]

/ba-g The errors are already reported in the issues mentioned above

[krwq]

@RenderMichael please resolve/fix the comments to move this forward, CI is currently blocking on that.

My biggest concerns are:

• APIs with IEnumerable<string>? - IMO they should not allow/return null but if this requires product changes we should file an issue instead.
• some tiny product changes in this PR - we should avoid them in this PR and do them separately - if we incidentally regress something it will be hard to pinpoint and possibly entire change will be reverted in such case

[danmoseley]

@RenderMichael are you still planning to continue with this PR? thanks!

[RenderMichael]

This PR is on my short list to cycle back to.

[danmoseley]

We have a "skill" for null reference annotation in the new dotnet/skills repo. If you're using Copilot CLI (or Claude Code I think) you introduce it like this

/plugin marketplace add dotnet/skills
/plugin marketplace browse
   --> shows "dotnet-agent-skills"
/plugin install dotnet-upgrade@dotnet-agent-skills

I tried an experiment, comparing your work here, with using Copilot to do the work without a skill, and using Copilot with a skill. Here's the writeup --

https://gist.github.com/danmoseley/2bd583fb655c9248828c0f8b339ac6c2

[danmoseley]

Updated gist above with two more experiments, using a more streamlined skill.

Outcome -- a skill slimmer than the one in dotnet/skills currently would likely work better. But, no version was successfully adding the NRT attributes: too much cross-method analysis, apparently. Seems the "fastest" workflow for changes like this would be to use AI, hopefully with the optimized skill (about 30 mins) then go through, add attributes and review the annotations it made. Likely this would be much quicker than doing it 100% human.

any thoughts welcome - hope you don't mind me piggy backing on your PR here

[Copilot]

Pull request overview

Copilot reviewed 37 out of 37 changed files in this pull request and generated 6 comments.

[Copilot]

Pull request overview

Copilot reviewed 37 out of 37 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

src/libraries/System.Diagnostics.EventLog/ref/System.Diagnostics.EventLog.cs:400

• In the ref assembly, EventLogSession.ExportLog* still requires a non-nullable string query (and ExportLogAndMessages requires non-nullable CultureInfo), but the implementation now accepts a nullable query (and nullable CultureInfo). Please update the ref nullable annotations to match the implementation so the public surface is consistent.

        public void ClearLog(string logName, string? backupPath) { }
        public void Dispose() { }
        protected virtual void Dispose(bool disposing) { }
        public void ExportLog(string path, System.Diagnostics.Eventing.Reader.PathType pathType, string query, string targetFilePath) { }
        public void ExportLog(string path, System.Diagnostics.Eventing.Reader.PathType pathType, string query, string targetFilePath, bool tolerateQueryErrors) { }
        public void ExportLogAndMessages(string path, System.Diagnostics.Eventing.Reader.PathType pathType, string query, string targetFilePath) { }
        public void ExportLogAndMessages(string path, System.Diagnostics.Eventing.Reader.PathType pathType, string query, string targetFilePath, bool tolerateQueryErrors, System.Globalization.CultureInfo targetCultureInfo) { }

[krwq]

@RenderMichael I've addressed my main piece of feedback and pushed to your branch

[krwq]

Any suggested product changes should be made separately for better visibility.

[krwq]

I've merged this as is as it's in acceptable state, we can iterate and improve further on this but that's also true to any other area

[krwq]

I've started copilot session to try to further iterate on all skipped product changes here. I'll post it if it looks reasonable

[krwq]

And thanks @RenderMichael for all the work here!