Probe the net9 Uri unescape bug rather than infer it from the target framework - #596
Merged
Conversation
…framework The tests added in #595 asserted that Uri.TryUnescapeDataString throws on net9.0 and net10.0, which bets that neither is ever serviced. dotnet/runtime#128610 is a written backport of the fix to release/10.0 that was deferred rather than rejected, so that bet could be lost, and net10.0 is in support until 2028. The behaviour is now probed once instead. net9.0 and net10.0 accept either outcome, and the assertion that it must not throw is kept for net11.0 and for every target where the polyfill is active.
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
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.
Follow-up to #595. Test-only; no change to shipped source, so
api_listand the API count are untouched.The problem
#595 pinned the known BCL bug with a compile-time guard:
That asserts a property of the runtime, using the target framework as the proxy. It holds today, but it is a bet that neither net9.0 nor net10.0 is ever serviced with the fix — and dotnet/runtime#128610 is a fully written backport to
release/10.0that was deferred, not rejected:net10.0 is LTS and in support until 2028-11-14, so that PR could be revived at any point. If it were, a patched net10.0 runtime would turn this repo's tests red for a reason that has nothing to do with Polyfill.
The change
The behaviour is probed once, and both the sizing test and the dedicated test key off the result:
Relaxing this must not quietly relax it for the polyfill too, which is the thing these tests actually exist to check. So the strict assertion is kept everywhere it can be:
net9.0 and net10.0 are the only frameworks permitted either outcome, and each is still asserted — a throw is asserted as a throw, and a non-throw is asserted to be
falsewith zero written. Any third behaviour, including a different exception type, still fails. On a serviced net10.0 the sizing test would additionally start covering the destination sizes it currently skips, rather than staying silent.Verification
Solution clean in Release, tests green on net11.0 (1697), net10.0 (1697), net9.0 (1697), net8.0 (1694), net462 (1659), plus PublicTests, EmbeddedTests, UnsafeTests and NoRefsTests.