Skip to content

Fix parsing inconsistencies between Uri ctors and TryCreate factories#123932

Open
MihaZupan wants to merge 6 commits intodotnet:mainfrom
MihaZupan:uri-sharedInit5
Open

Fix parsing inconsistencies between Uri ctors and TryCreate factories#123932
MihaZupan wants to merge 6 commits intodotnet:mainfrom
MihaZupan:uri-sharedInit5

Conversation

@MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Feb 3, 2026

This is the "future change" I was referring to in #122001 and #122002.

Uri ctors and TryCreate factories used almost the same logic, but the duplication introduced a slight difference in how the fallback to relative Uris was handled.
When using TryCreate, we would skip the iri normalization. E.g. compare

var uri1 = new Uri("p%41th", UriKind.Relative);
Uri.TryCreate("p%41th", UriKind.Relative, out Uri uri2);
Console.WriteLine(uri1); // pAth
Console.WriteLine(uri2); // p%41th

01abba3 avoids the code duplication and makes the behavior between the two the same.


The other inconsistency was around how the fallback to relative Uris worked for various cases where we decide we can't create an Absolute one.
If we were falling back from an implicit file path, or an invalid absolute Uri with a valid scheme, we would skip iri normalization.

d50ad4f uses the same logic for all such cases.


The distinction doesn't matter much in practice if you end up combining the Uri with a base, they'll get normalized the same. We could also choose to make the break in the other direction if we wanted to - not do iri normalization for relative uris at all.

It only matters if you interrogate the relative Uri directly. Marking as breaking-change regardless so that we remember to call it out in docs.

@MihaZupan MihaZupan added this to the 11.0.0 milestone Feb 3, 2026
@MihaZupan MihaZupan self-assigned this Feb 3, 2026
@MihaZupan MihaZupan added area-System.Net breaking-change Issue or PR that represents a breaking API or functional change over a previous release. labels Feb 3, 2026
@dotnet-policy-service dotnet-policy-service bot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Feb 3, 2026
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Copilot AI review requested due to automatic review settings February 10, 2026 00:14
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Aligns Uri constructors and Uri.TryCreate factory methods by removing duplicated initialization logic and ensuring consistent IRI normalization behavior when falling back to relative URIs.

Changes:

  • Refactors URI initialization so constructors and TryCreate share a single core creation path (TryCreateThis).
  • Ensures fallback-to-relative paths apply consistent IRI normalization.
  • Expands ToString functional test data and adds paired coverage for ctor vs TryCreate.

Reviewed changes

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

File Description
src/libraries/System.Private.Uri/src/System/UriExt.cs Refactors shared URI construction logic to unify ctor and TryCreate behavior and normalize relative-fallback handling.
src/libraries/System.Private.Uri/tests/FunctionalTests/UriTests.cs Adds/extends ToString test cases and mirrors them through TryCreate to validate consistency.

@stephentoub
Copy link
Member

🤖 Copilot Code Review — PR #123932

Holistic Assessment

Motivation: The PR addresses a real inconsistency where Uri constructors and Uri.TryCreate factories produced different results for relative URIs containing Unicode or percent-encoded unreserved characters. The problem is clearly demonstrated: new Uri("p%41th", UriKind.Relative) yields "pAth" while Uri.TryCreate yielded "p%41th". This is a legitimate fix.

Approach: The approach is sound — unify the code paths by introducing a shared TryCreateThis method that both CreateThis (used by constructors) and CreateHelper (used by TryCreate) call. The SwitchToRelativeUri label consolidates all fallback-to-relative paths to ensure consistent IRI normalization.

Summary: ✅ LGTM. The refactoring is clean, correct, and well-tested. The code eliminates duplication while ensuring behavioral consistency between constructors and TryCreate factories. The breaking change is appropriately labeled and documented.


Detailed Findings

✅ Code Unification — Correctly implemented

The refactoring successfully unifies the constructor and TryCreate code paths:

  • CreateThis (lines 18-26) now simply calls TryCreateThis and throws if it returns an error.
  • CreateHelper (lines 635-653) creates a new Uri() and calls TryCreateThis, returning null on error.
  • The SwitchToRelativeUri label (lines 155-168) consolidates all relative URI fallback paths, ensuring IRI normalization is consistently applied when hasUnicode is true.

This eliminates the previous inconsistency where some fallback paths (like implicit file paths or simple syntax errors) did not apply IRI normalization.

✅ Parameterless Constructor — Appropriately scoped

The new private Uri() constructor (lines 629-632) is correctly:

  • Marked private
  • Documented as "Must never be used except by CreateHelper"
  • Protected by #pragma warning disable CS8618 with a clear comment about why _string is initialized later

This prevents misuse while enabling the unified code path.

✅ Debug.Assert in CreateHelper — Correct

The assertion at line 650:

Debug.Assert(result.Syntax is { IsSimple: false }, "A custom UriParser threw on InitializeAndValidate.");

This correctly asserts that built-in simple parsers should never throw UriFormatException (they return errors via the return value), while custom parsers may throw, which is then caught and results in returning null. The pattern matches the pre-existing behavior.

✅ Test Coverage — Adequate

The test expansion in UriTests.cs (lines 917-948):

  • Tests relative URIs with percent-encoded characters (p%41thpAth)
  • Tests relative URIs with Unicode characters (pa\uFFFFthpa%EF%BF%BFth)
  • Tests various path formats (DOS paths, UNC-like paths, scheme-like relative paths)
  • Each test case is mirrored through Uri.TryCreate to verify both paths produce the same result
  • Uses Assert.Same(uri.OriginalString, uri2.OriginalString) to verify the original strings are preserved identically

✅ Thread Safety — No concerns

The code is thread-safe:

  • CreateHelper creates a local Uri instance that is not exposed until fully initialized
  • No shared mutable state is introduced
  • The TryCreateThis method operates only on instance fields of the newly created object

✅ Breaking Change — Appropriately documented

The PR is correctly labeled with breaking-change and needs-breaking-change-doc-created. The change makes TryCreate consistent with constructors for relative URIs, which is the correct direction for this fix.

💡 Suggestion — Minor typo fix already included

Commit 5 (1f7e6d1) fixes "constructos" → "constructors" in the XML doc comment at line 16. Good attention to detail.


Models consulted: Claude Sonnet 4, Gemini 3 Pro (GPT-5.1-Codex timed out after 10 minutes)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Net breaking-change Issue or PR that represents a breaking API or functional change over a previous release. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants