[release/11.0] Make the Nullable<T>.Value exception message actionable - #132379
Open
github-actions[bot] wants to merge 1 commit into
Open
[release/11.0] Make the Nullable<T>.Value exception message actionable#132379github-actions[bot] wants to merge 1 commit into
github-actions[bot] wants to merge 1 commit into
Conversation
Fixes #126560 ## Problem `InvalidOperation_NoValue` — the message thrown by `Nullable<T>.Value` when there's no value — reads: > Nullable object must have a value. As #126560 points out, this states an obligation on the *object's state* rather than naming the *invalid action*. It reads as "you must assign a value", when the actual fix is almost always the opposite: don't read `.Value` without checking `.HasValue` first. It also never mentions which member was accessed. ## Change ```diff -Nullable object must have a value. +Cannot read the Value property of a Nullable object that has no value. Check HasValue before reading Value. ``` This follows the wording style already used in the repo for comparable "you called this on something that has no value" failures: - `SqlMisc_NullValueMessage` (`System.Data.Common`) — *"Data is Null. This method or property cannot be called on Null values."* - `ReflectionModel_ExportNotReadable` (`System.ComponentModel.Composition`) — *"Cannot get the value of property '{0}', because the member is not readable. The property must have an accessible getter."* - `NoResultOnFailed` (`System.Text.RegularExpressions`) — *"Result cannot be called on a failed Match."* The issue also floated "Cannot read the Value property of a null Nullable object." I avoided "null Nullable object" because `Nullable<T>` is a struct and is never `null` — "that has no value" keeps the same cadence without the imprecision. The second sentence names the check to perform, per *"DO ensure that exception messages are clear and actionable"* (`docs/coding-guidelines/framework-design-guidelines-digest.md`). The resource **name** is unchanged. ## Scope `InvalidOperation_NoValue` is not shared with any unrelated call site — all definitions and references are `Nullable<T>.Value` semantics: | | Path | |---|---| | def | `src/libraries/System.Private.CoreLib/src/Resources/Strings.resx` | | def | `src/coreclr/nativeaot/System.Private.Reflection.Execution/src/Resources/Strings.resx` | | use | `ThrowHelper.ThrowInvalidOperationException_InvalidOperation_NoValue` — sole caller is the `Nullable<T>.Value` getter | | use | `CustomMethodMapper.Nullable.cs` — NativeAOT reflection-invoke shim for `Nullable<T>.Value` | Both `.resx` definitions are updated. Mono shares the libraries CoreLib `.resx`; there is no third copy. No test asserts the text, and the NativeAOT `FrameworkStrings` string-pinning smoke test pins other resources. Per `docs/coding-guidelines/breaking-change-rules.md`, changing the text of an error message is not a breaking change ("users should not rely on these text messages, and they change anyways based on culture"). ## Validation - Built `System.Private.CoreLib` (`build.cmd clr.corelib -c Release`) — clean. - Confirmed against the produced `System.Private.CoreLib.dll` that the new string is embedded and the old one is gone, so `SR.InvalidOperation_NoValue` still resolves. - The NativeAOT `System.Private.Reflection.Execution` `.resx` was not compiled locally — that project currently fails on a pre-existing missing generated `AsmOffsets.cs` requiring a full NativeAOT native build. The edit there is identical in shape and doesn't change the resource name. CI covers it. > [!NOTE] > This pull request description was generated by GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @steveisok, @dotnet/area-system-reflection |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #132367 to release/11.0 (for .NET 11 RC2)
/cc @jeffhandley
Customer Impact
Reported in #126560.
Nullable<T>.ValuethrowsInvalidOperationExceptionwith "Nullable object must have a value." That describes an obligation on the object's state rather than the invalid action, never names the member that was accessed, and doesn't mentionHasValue. The reporter's point is that it reads as "you must assign a value", when the fix is normally to checkHasValuebefore readingValue.New text: "Cannot read the Value property of a Nullable object that has no value. Check HasValue before reading Value."
Diagnostics only; no functional impact, but addressing a customer-reported pitfall.
Regression
The original wording predates .NET Core.
Testing
Covered by CI on #132367. No test asserts this message text; verified by searching the repo, including the NativeAOT
FrameworkStringsstring-pinning test, which pins other resources. No tests were added: the change is a resource value with no behavior to exercise beyond the string itself. Locally confirmed that the builtSystem.Private.CoreLibcontains the new string and no longer contains the old one.Risk
Low. The functional change is a single resource value in
src/libraries/System.Private.CoreLib/src/Resources/Strings.resx. The resource name is unchanged, so there is no code change, no API change, and no behavior change other than the message text.This backport also carries the cleanup that came out of review on #132367: 15 unreferenced strings removed from the NativeAOT
System.Private.Reflection.ExecutionStrings.resx(including a dead copy ofInvalidOperation_NoValue) and the unusedHashHelpers.cscompile item. All were verified unreferenced and have no runtime effect, but if a minimal backport diff is preferred, those commits can be dropped and only the exception message change taken.