Fix parsing inconsistencies between Uri ctors and TryCreate factories (dotnet#123932)

MihaZupan · richlander · commit 6db8fa34bf77 · 2026-02-13T21:40:28.000-08:00
diff --git a/src/libraries/System.Private.Uri/src/System/Uri.cs b/src/libraries/System.Private.Uri/src/System/Uri.cs
@@ -145,6 +145,8 @@ internal enum Flags : ulong
         [Conditional("DEBUG")]
         private void DebugSetLeftCtor()
         {
+            DebugAssertInCtor();
+
             _flags |= Flags.Debug_LeftConstructor;
 
             AssertInvariants();
@@ -424,7 +426,6 @@ public Uri([StringSyntax(StringSyntaxAttribute.Uri)] string uriString)
             ArgumentNullException.ThrowIfNull(uriString);
 
             CreateThis(uriString, false, UriKind.Absolute);
-            DebugSetLeftCtor();
         }
 
         //
@@ -438,7 +439,6 @@ public Uri([StringSyntax(StringSyntaxAttribute.Uri)] string uriString, bool dont
             ArgumentNullException.ThrowIfNull(uriString);
 
             CreateThis(uriString, dontEscape, UriKind.Absolute);
-            DebugSetLeftCtor();
         }
 
         //
@@ -456,7 +456,6 @@ public Uri(Uri baseUri, string? relativeUri, bool dontEscape)
                 throw new ArgumentOutOfRangeException(nameof(baseUri));
 
             CreateUri(baseUri, relativeUri, dontEscape);
-            DebugSetLeftCtor();
         }
 
         //
@@ -467,7 +466,6 @@ public Uri([StringSyntax(StringSyntaxAttribute.Uri, nameof(uriKind))] string uri
             ArgumentNullException.ThrowIfNull(uriString);
 
             CreateThis(uriString, false, uriKind);
-            DebugSetLeftCtor();
         }
 
         /// <summary>
@@ -480,7 +478,6 @@ public Uri([StringSyntax(StringSyntaxAttribute.Uri)] string uriString, in UriCre
             ArgumentNullException.ThrowIfNull(uriString);
 
             CreateThis(uriString, false, UriKind.Absolute, in creationOptions);
-            DebugSetLeftCtor();
         }
 
         //
@@ -498,7 +495,6 @@ public Uri(Uri baseUri, string? relativeUri)
                 throw new ArgumentOutOfRangeException(nameof(baseUri));
 
             CreateUri(baseUri, relativeUri, false);
-            DebugSetLeftCtor();
         }
 
         //
@@ -515,7 +511,6 @@ protected Uri(SerializationInfo serializationInfo, StreamingContext streamingCon
             if (uriString!.Length != 0)
             {
                 CreateThis(uriString, false, UriKind.Absolute);
-                DebugSetLeftCtor();
                 return;
             }
 
@@ -524,7 +519,6 @@ protected Uri(SerializationInfo serializationInfo, StreamingContext streamingCon
                 throw new ArgumentException(SR.Format(SR.InvalidNullArgument, "RelativeUri"), nameof(serializationInfo));
 
             CreateThis(uriString, false, UriKind.Relative);
-            DebugSetLeftCtor();
         }
 
         //
@@ -617,7 +611,6 @@ public Uri(Uri baseUri, Uri relativeUri)
                     if (!ReferenceEquals(this, resolvedRelativeUri))
                         CreateThisFromUri(resolvedRelativeUri);
 
-                    DebugSetLeftCtor();
                     return;
                 }
             }
@@ -634,7 +627,6 @@ public Uri(Uri baseUri, Uri relativeUri)
             _syntax = null!;
             _originalUnicodeString = null!;
             CreateThis(newUriString, dontEscape, UriKind.Absolute);
-            DebugSetLeftCtor();
         }
 
         //
diff --git a/src/libraries/System.Private.Uri/src/System/UriExt.cs b/src/libraries/System.Private.Uri/src/System/UriExt.cs
@@ -13,11 +13,21 @@ namespace System
 {
     public partial class Uri
     {
-        //
-        // All public ctors go through here
-        //
+        /// <summary>Helper called by all constructors to construct the Uri.</summary>
         [MemberNotNull(nameof(_string))]
         private void CreateThis(string? uri, bool dontEscape, UriKind uriKind, in UriCreationOptions creationOptions = default)
+        {
+            UriFormatException? e = TryCreateThis(uri, dontEscape, uriKind, in creationOptions);
+
+            if (e is not null)
+            {
+                throw e;
+            }
+        }
+
+        /// <summary>Core helper called by all constructors and TryCreate factories to construct the Uri.</summary>
+        [MemberNotNull(nameof(_string))]
+        private UriFormatException? TryCreateThis(string? uri, bool dontEscape, UriKind uriKind, in UriCreationOptions creationOptions = default)
         {
             DebugAssertInCtor();
 
@@ -37,15 +47,6 @@ private void CreateThis(string? uri, bool dontEscape, UriKind uriKind, in UriCre
                 _flags |= Flags.DisablePathAndQueryCanonicalization;
 
             ParsingError err = ParseScheme(_string, ref _flags, ref _syntax!);
-
-            UriFormatException? e = InitializeUri(err, uriKind);
-            if (e != null)
-                throw e;
-        }
-
-        private UriFormatException? InitializeUri(ParsingError err, UriKind uriKind)
-        {
-            DebugAssertInCtor();
             Debug.Assert((err is ParsingError.None) == (_syntax is not null));
 
             bool hasUnicode = false;
@@ -63,19 +64,10 @@ private void CreateThis(string? uri, bool dontEscape, UriKind uriKind, in UriCre
                 // and we'll allow relative Uri's, then create one.
                 if (uriKind != UriKind.Absolute && err <= ParsingError.LastErrorOkayForRelativeUris)
                 {
-                    _flags &= Flags.UserEscaped | Flags.HasUnicode; // the only flags that makes sense for a relative uri
-                    if (hasUnicode)
-                    {
-                        // Iri'ze and then normalize relative uris
-                        var vsb = new ValueStringBuilder(stackalloc char[StackallocThreshold]);
-                        IriHelper.EscapeUnescapeIri(ref vsb, _originalUnicodeString, isQuery: false);
-                        _string = vsb.ToString();
-                    }
-                    return null;
+                    goto SwitchToRelativeUri;
                 }
 
                 // This is a fatal error based solely on scheme name parsing
-                _string = null!; // make it be invalid Uri
                 return GetException(err);
             }
 
@@ -85,9 +77,7 @@ private void CreateThis(string? uri, bool dontEscape, UriKind uriKind, in UriCre
             {
                 if (uriKind == UriKind.Relative)
                 {
-                    _syntax = null!; // make it be relative Uri
-                    _flags &= Flags.UserEscaped; // the only flag that makes sense for a relative uri
-                    return null;
+                    goto SwitchToRelativeUri;
                 }
 
                 // V1 compat
@@ -98,9 +88,7 @@ private void CreateThis(string? uri, bool dontEscape, UriKind uriKind, in UriCre
                    ((_string.Length >= 2 && (_string[0] != '\\' || _string[1] != '\\'))
                     || (!OperatingSystem.IsWindows() && InFact(Flags.UnixPath))))
                 {
-                    _syntax = null!; //make it be relative Uri
-                    _flags &= Flags.UserEscaped; // the only flag that makes sense for a relative uri
-                    return null;
+                    goto SwitchToRelativeUri;
                     // Otherwise an absolute file Uri wins when it's of the form "\\something"
                 }
             }
@@ -112,9 +100,7 @@ private void CreateThis(string? uri, bool dontEscape, UriKind uriKind, in UriCre
                     if (uriKind != UriKind.Absolute && err <= ParsingError.LastErrorOkayForRelativeUris)
                     {
                         // RFC 3986 Section 5.4.2 - http:(relativeUri) may be considered a valid relative Uri.
-                        _syntax = null!; // convert to relative uri
-                        _flags &= Flags.UserEscaped; // the only flag that makes sense for a relative uri
-                        return null;
+                        goto SwitchToRelativeUri;
                     }
 
                     return GetException(err);
@@ -164,6 +150,23 @@ private void CreateThis(string? uri, bool dontEscape, UriKind uriKind, in UriCre
             }
 
             // We have a valid absolute Uri.
+            DebugSetLeftCtor();
+            return null;
+
+        SwitchToRelativeUri:
+            Debug.Assert(uriKind != UriKind.Absolute);
+
+            _syntax = null!;
+            _flags &= Flags.UserEscaped | Flags.HasUnicode; // the only flags that make sense for a relative uri
+
+            if (hasUnicode)
+            {
+                var vsb = new ValueStringBuilder(stackalloc char[StackallocThreshold]);
+                IriHelper.EscapeUnescapeIri(ref vsb, _originalUnicodeString, isQuery: false);
+                _string = vsb.ToString();
+            }
+
+            DebugSetLeftCtor();
             return null;
         }
 
@@ -220,7 +223,6 @@ private static bool CheckForUnicodeOrEscapedUnreserved(string data)
         public static bool TryCreate([NotNullWhen(true), StringSyntax(StringSyntaxAttribute.Uri, "uriKind")] string? uriString, UriKind uriKind, [NotNullWhen(true)] out Uri? result)
         {
             result = CreateHelper(uriString, false, uriKind);
-            result?.DebugSetLeftCtor();
             return result is not null;
         }
 
@@ -234,7 +236,6 @@ public static bool TryCreate([NotNullWhen(true), StringSyntax(StringSyntaxAttrib
         public static bool TryCreate([NotNullWhen(true), StringSyntax(StringSyntaxAttribute.Uri)] string? uriString, in UriCreationOptions creationOptions, [NotNullWhen(true)] out Uri? result)
         {
             result = CreateHelper(uriString, false, UriKind.Absolute, in creationOptions);
-            result?.DebugSetLeftCtor();
             return result is not null;
         }
 
@@ -283,7 +284,6 @@ public static bool TryCreate(Uri? baseUri, Uri? relativeUri, [NotNullWhen(true)]
             result ??= CreateHelper(newUriString!, dontEscape, UriKind.Absolute);
             Debug.Assert(result is null || result.IsAbsoluteUri);
 
-            result?.DebugSetLeftCtor();
             return result is not null;
         }
 
@@ -625,77 +625,28 @@ public static string EscapeDataString(ReadOnlySpan<char> charsToEscape) =>
         public static bool TryEscapeDataString(ReadOnlySpan<char> charsToEscape, Span<char> destination, out int charsWritten) =>
             UriHelper.TryEscapeDataString(charsToEscape, destination, out charsWritten);
 
-        // Should never be used except by the below method
-        private Uri(Flags flags, UriParser? uriParser, string uri)
-        {
-            _flags = flags;
-            _syntax = uriParser!;
-            _string = uri;
-
-            if (uriParser is null)
-            {
-                // Relative Uris are fully initialized after the call to this constructor
-                // Absolute Uris will be initialized with a call to InitializeUri on the newly created instance
-                DebugSetLeftCtor();
-            }
-        }
+#pragma warning disable CS8618 // _string will be initialized by TryCreateThis later.
+        /// <summary>Must never be used except by <see cref="CreateHelper(string?, bool, UriKind, in UriCreationOptions)"/>.</summary>
+        private Uri() { }
+#pragma warning restore CS8618
 
-        //
-        // a Uri.TryCreate() method goes through here.
-        //
+        /// <summary>Called by TryCreate.</summary>
         internal static Uri? CreateHelper(string? uriString, bool dontEscape, UriKind uriKind, in UriCreationOptions creationOptions = default)
         {
             if (uriString is null)
             {
                 return null;
             }
 
-            if (uriKind is < UriKind.RelativeOrAbsolute or > UriKind.Relative)
-            {
-                throw new ArgumentException(SR.Format(SR.net_uri_InvalidUriKind, uriKind));
-            }
-
-            UriParser? syntax = null;
-            Flags flags = Flags.Zero;
-            ParsingError err = ParseScheme(uriString, ref flags, ref syntax);
-
-            if (dontEscape)
-                flags |= Flags.UserEscaped;
-
-            if (creationOptions.DangerousDisablePathAndQueryCanonicalization)
-                flags |= Flags.DisablePathAndQueryCanonicalization;
-
-            // We won't use User factory for these errors
-            if (err != ParsingError.None)
-            {
-                // If it looks as a relative Uri, custom factory is ignored
-                if (uriKind != UriKind.Absolute && err <= ParsingError.LastErrorOkayForRelativeUris)
-                    return new Uri((flags & Flags.UserEscaped), null, uriString);
-
-                return null;
-            }
-
-            // Cannot be relative Uri if came here
-            Debug.Assert(syntax != null);
-            Uri result = new Uri(flags, syntax, uriString);
-
-            // Validate instance using ether built in or a user Parser
+            Uri result = new();
             try
             {
-                UriFormatException? e = result.InitializeUri(err, uriKind);
-
-                if (e == null)
-                {
-                    result.DebugSetLeftCtor();
-                    return result;
-                }
-
-                return null;
+                UriFormatException? e = result.TryCreateThis(uriString, dontEscape, uriKind, in creationOptions);
+                return e is null ? result : null;
             }
             catch (UriFormatException)
             {
-                Debug.Assert(!syntax.IsSimple, "A UriPraser threw on InitializeAndValidate.");
-                // A precaution since custom Parser should never throw in this case.
+                Debug.Assert(result.Syntax is { IsSimple: false }, "A custom UriParser threw on InitializeAndValidate.");
                 return null;
             }
         }
@@ -938,6 +889,7 @@ internal bool IsBaseOfHelper(Uri uriLink)
         private void CreateThisFromUri(Uri otherUri)
         {
             DebugAssertInCtor();
+            Debug.Assert((otherUri._flags & Flags.Debug_LeftConstructor) != 0);
 
             _flags = otherUri._flags;
 
diff --git a/src/libraries/System.Private.Uri/tests/FunctionalTests/UriTests.cs b/src/libraries/System.Private.Uri/tests/FunctionalTests/UriTests.cs
@@ -914,14 +914,38 @@ public static IEnumerable<object[]> ToStringTest_MemberData()
             // a) We can test each method without it being impacted by implicit caching of a previous method's results
             // b) xunit's implicit formatting of arguments doesn't similarly disturb the results
 
-            yield return new object[] { () => new Uri("http://test"), "http://test/" };
-            yield return new object[] { () => new Uri("   http://test   "), "http://test/" };
-            yield return new object[] { () => new Uri("/test", UriKind.Relative), "/test" };
-            yield return new object[] { () => new Uri("test", UriKind.Relative), "test" };
-            yield return new object[] { () => new Uri("http://foo/bar/baz#frag"), "http://foo/bar/baz#frag" };
-            yield return new object[] { () => new Uri(new Uri(@"http://www.contoso.com/"), "catalog/shownew.htm?date=today"), "http://www.contoso.com/catalog/shownew.htm?date=today" };
-            yield return new object[] { () => new Uri("http://test/a/b/c/d/../../e/f"), "http://test/a/b/e/f" };
-            yield return new object[] { () => { var uri = new Uri("http://test/a/b/c/d/../../e/f"); uri.ToString(); return uri; }, "http://test/a/b/e/f" };
+            (Func<Uri>, string)[] testData =
+            [
+                (() => new Uri("http://test"), "http://test/"),
+                (() => new Uri("   http://test   "), "http://test/"),
+                (() => new Uri("/test", UriKind.Relative), "/test"),
+                (() => new Uri("test", UriKind.Relative), "test"),
+                (() => new Uri("http://foo/bar/baz#frag"), "http://foo/bar/baz#frag"),
+                (() => new Uri(new Uri(@"http://www.contoso.com/"), "catalog/shownew.htm?date=today"), "http://www.contoso.com/catalog/shownew.htm?date=today"),
+                (() => new Uri("http://test/a/b/c/d/../../e/f"), "http://test/a/b/e/f"),
+                (() => { var uri = new Uri("http://test/a/b/c/d/../../e/f"); uri.ToString(); return uri; }, "http://test/a/b/e/f"),
+                (() => new Uri("p%41th", UriKind.Relative), "pAth"),
+                (() => new Uri("pa\uFFFFth", UriKind.Relative), "pa%EF%BF%BFth"),
+                (() => new Uri("C:\\path", UriKind.Relative), "C:\\path"),
+                (() => new Uri("C:\\p%41th", UriKind.Relative), "C:\\pAth"),
+                (() => new Uri("http:\\host/path", UriKind.Relative), "http:\\host/path"),
+                (() => new Uri("http:\\host/p%41th", UriKind.Relative), "http:\\host/pAth"),
+                (() => new Uri("//host/path", UriKind.RelativeOrAbsolute), "//host/path"),
+                (() => new Uri("//host/p%41th", UriKind.RelativeOrAbsolute), "//host/pAth"),
+            ];
+
+            foreach ((Func<Uri> factory, string expected) in testData)
+            {
+                yield return [factory, expected];
+
+                yield return [() =>
+                {
+                    Uri uri = factory();
+                    Assert.True(Uri.TryCreate(uri.OriginalString, uri.IsAbsoluteUri ? UriKind.Absolute : UriKind.Relative, out Uri? uri2));
+                    Assert.Same(uri.OriginalString, uri2.OriginalString);
+                    return uri2;
+                }, expected];
+            }
         }
 
         [Theory]

Original file line number	Diff line number	Diff line change
`@@ -145,6 +145,8 @@ internal enum Flags : ulong`
`145`	`145`	`[Conditional("DEBUG")]`
`146`	`146`	`private void DebugSetLeftCtor()`
`147`	`147`	`{`
	`148`	`+ DebugAssertInCtor();`
	`149`	`+`
`148`	`150`	`_flags \|= Flags.Debug_LeftConstructor;`
`149`	`151`
`150`	`152`	`AssertInvariants();`
`@@ -424,7 +426,6 @@ public Uri([StringSyntax(StringSyntaxAttribute.Uri)] string uriString)`
`424`	`426`	`ArgumentNullException.ThrowIfNull(uriString);`
`425`	`427`
`426`	`428`	`CreateThis(uriString, false, UriKind.Absolute);`
`427`		`- DebugSetLeftCtor();`
`428`	`429`	`}`
`429`	`430`
`430`	`431`	`//`
`@@ -438,7 +439,6 @@ public Uri([StringSyntax(StringSyntaxAttribute.Uri)] string uriString, bool dont`
`438`	`439`	`ArgumentNullException.ThrowIfNull(uriString);`
`439`	`440`
`440`	`441`	`CreateThis(uriString, dontEscape, UriKind.Absolute);`
`441`		`- DebugSetLeftCtor();`
`442`	`442`	`}`
`443`	`443`
`444`	`444`	`//`
`@@ -456,7 +456,6 @@ public Uri(Uri baseUri, string? relativeUri, bool dontEscape)`
`456`	`456`	`throw new ArgumentOutOfRangeException(nameof(baseUri));`
`457`	`457`
`458`	`458`	`CreateUri(baseUri, relativeUri, dontEscape);`
`459`		`- DebugSetLeftCtor();`
`460`	`459`	`}`
`461`	`460`
`462`	`461`	`//`
`@@ -467,7 +466,6 @@ public Uri([StringSyntax(StringSyntaxAttribute.Uri, nameof(uriKind))] string uri`
`467`	`466`	`ArgumentNullException.ThrowIfNull(uriString);`
`468`	`467`
`469`	`468`	`CreateThis(uriString, false, uriKind);`
`470`		`- DebugSetLeftCtor();`
`471`	`469`	`}`
`472`	`470`
`473`	`471`	`/// <summary>`
`@@ -480,7 +478,6 @@ public Uri([StringSyntax(StringSyntaxAttribute.Uri)] string uriString, in UriCre`
`480`	`478`	`ArgumentNullException.ThrowIfNull(uriString);`
`481`	`479`
`482`	`480`	`CreateThis(uriString, false, UriKind.Absolute, in creationOptions);`
`483`		`- DebugSetLeftCtor();`
`484`	`481`	`}`
`485`	`482`
`486`	`483`	`//`
`@@ -498,7 +495,6 @@ public Uri(Uri baseUri, string? relativeUri)`
`498`	`495`	`throw new ArgumentOutOfRangeException(nameof(baseUri));`
`499`	`496`
`500`	`497`	`CreateUri(baseUri, relativeUri, false);`
`501`		`- DebugSetLeftCtor();`
`502`	`498`	`}`
`503`	`499`
`504`	`500`	`//`
`@@ -515,7 +511,6 @@ protected Uri(SerializationInfo serializationInfo, StreamingContext streamingCon`
`515`	`511`	`if (uriString!.Length != 0)`
`516`	`512`	`{`
`517`	`513`	`CreateThis(uriString, false, UriKind.Absolute);`
`518`		`- DebugSetLeftCtor();`
`519`	`514`	`return;`
`520`	`515`	`}`
`521`	`516`
`@@ -524,7 +519,6 @@ protected Uri(SerializationInfo serializationInfo, StreamingContext streamingCon`
`524`	`519`	`throw new ArgumentException(SR.Format(SR.InvalidNullArgument, "RelativeUri"), nameof(serializationInfo));`
`525`	`520`
`526`	`521`	`CreateThis(uriString, false, UriKind.Relative);`
`527`		`- DebugSetLeftCtor();`
`528`	`522`	`}`
`529`	`523`
`530`	`524`	`//`
`@@ -617,7 +611,6 @@ public Uri(Uri baseUri, Uri relativeUri)`
`617`	`611`	`if (!ReferenceEquals(this, resolvedRelativeUri))`
`618`	`612`	`CreateThisFromUri(resolvedRelativeUri);`
`619`	`613`
`620`		`- DebugSetLeftCtor();`
`621`	`614`	`return;`
`622`	`615`	`}`
`623`	`616`	`}`
`@@ -634,7 +627,6 @@ public Uri(Uri baseUri, Uri relativeUri)`
`634`	`627`	`_syntax = null!;`
`635`	`628`	`_originalUnicodeString = null!;`
`636`	`629`	`CreateThis(newUriString, dontEscape, UriKind.Absolute);`
`637`		`- DebugSetLeftCtor();`
`638`	`630`	`}`
`639`	`631`
`640`	`632`	`//`