Skip to content

Preserve nullable struct null-check semantics in UA0003#4039

Open
marcschier wants to merge 2 commits into
OPCFoundation:masterfrom
marcschier:marcschier/review-fix/ua0003-nullable
Open

Preserve nullable struct null-check semantics in UA0003#4039
marcschier wants to merge 2 commits into
OPCFoundation:masterfrom
marcschier:marcschier/review-fix/ua0003-nullable

Conversation

@marcschier

Copy link
Copy Markdown
Collaborator

Failure

UA0003 unwrapped Nullable<T> before classifying OPC UA built-in structs. That
reported valid nullable == null and != null checks and offered fixes whose
.IsNull semantics differ from nullable-value absence.

Fix

  • Leave System.Nullable<T> comparisons unchanged.
  • Continue reporting direct null comparisons on OPC UA structs.
  • Add coverage for nullable NodeId and LocalizedText operand orderings.

Reference

Tests

  • dotnet test tests\Opc.Ua.MigrationAnalyzer.Tests\Opc.Ua.MigrationAnalyzer.Tests.csproj -c Release -f net10.0 --nologo (121 passed)
  • dotnet test tests\Opc.Ua.MigrationAnalyzer.Tests\Opc.Ua.MigrationAnalyzer.Tests.csproj -c Release -f net48 --nologo (121 passed)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@marcschier
marcschier marked this pull request as ready for review July 19, 2026 10:43
Copilot AI review requested due to automatic review settings July 19, 2026 10:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Adjusts UA0003 to preserve C# nullable semantics by no longer unwrapping Nullable<T> before deciding whether a null-comparison should be reported/fixed.

Changes:

  • Stop unwrapping Nullable<T> in UA0003 analysis, returning early to keep == null / != null checks intact.
  • Add tests to ensure UA0003 does not report null-comparisons for nullable NodeId and LocalizedText (including operand ordering).

Reviewed changes

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

File Description
tools/Opc.Ua.MigrationAnalyzer/Analyzers/UA0003NullCheckOnStructTypeAnalyzer.cs Changes analysis to skip nullable types, preserving Nullable<T> null-check behavior.
tests/Opc.Ua.MigrationAnalyzer.Tests/Analyzers/UA0003Tests.cs Adds regression coverage ensuring nullable UA structs are not flagged, including null != value ordering.

Comment thread tools/Opc.Ua.MigrationAnalyzer/Analyzers/UA0003NullCheckOnStructTypeAnalyzer.cs Outdated
Comment thread tests/Opc.Ua.MigrationAnalyzer.Tests/Analyzers/UA0003Tests.cs Outdated
@marcschier
marcschier marked this pull request as draft July 19, 2026 11:35
@marcschier
marcschier marked this pull request as ready for review July 19, 2026 11:54
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@marcschier
marcschier marked this pull request as draft July 19, 2026 16:21
@marcschier
marcschier marked this pull request as ready for review July 19, 2026 16:24
@marcschier
marcschier marked this pull request as draft July 19, 2026 16:26
@marcschier
marcschier marked this pull request as ready for review July 19, 2026 16:30
@marcschier
marcschier requested a review from romanett July 19, 2026 18:37
@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.54%. Comparing base (70c74e7) to head (e15c475).
⚠️ Report is 8 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4039      +/-   ##
==========================================
- Coverage   73.55%   73.54%   -0.02%     
==========================================
  Files        1345     1345              
  Lines      180036   180037       +1     
  Branches    31677    31678       +1     
==========================================
- Hits       132427   132404      -23     
- Misses      36911    36922      +11     
- Partials    10698    10711      +13     
Files with missing lines Coverage Δ
...r/Analyzers/UA0003NullCheckOnStructTypeAnalyzer.cs 90.47% <100.00%> (+4.42%) ⬆️

... and 21 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@marcschier

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@marcschier marcschier added the ready Ready to merge once CI Passes label Jul 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready Ready to merge once CI Passes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants