Skip to content

Conversation

@MihaZupan
Copy link
Member

The current initialization logic is written in a way that makes it hard to read, and hides inconsistencise / unreachable logic.

This PR only shuffles things around & removes some unreachable branches without changing the behavior.
The aim is to make future changes that do change behavior actually reviewable.

Some context:

  • we use _syntax to represent the Uri scheme. It's always null for relative Uris and set for absolute ones
  • before we call into the InitializeUri we try to parse the scheme. This populates both ParsingError err and _syntax. These two are mutually exclusive - if err is set it means that _syntax == null and vice-versa.

I've split this up into smaller commits:

  • a6523bb?w=1
    • This recognizes the assertion I made above about the error and syntax being mutually exclusive.
    • It moves the logic for err != ParsingError.None up to the top, allowing us to drop the extra nesting of the if (_syntax != null) branch below it (it'll always be true).
    • It also removes two dead branches that were checking if err != ParsingError.None when we know it can't be
  • c73795e
    • This simplifies our logic that decides whether an implicit file should be trated as absolute or relative.
    • We were effectively checking NotAny(Flags.DosPath) && uriKind == UriKind.Relative in one place and InFact(Flags.DosPath) && uriKind == UriKind.Relative in another, while doing the same thing in both cases. I simplified that condition to just check for UriKind.Relative.
    • After you handle the Relative case, the uriKind != UriKind.Absolute check becomes == UriKind.RelativeOrAbsolute, which makes it clearer what we're doing
  • fdcd584
    • Changes the UriFormatException to the return value instead of an out param
  • 7400e9a
    • Takes a block of code that's duplicated at the end of both the if and else and moves it after the branch
  • 393f78f
    • Inverts one conditon to drop some nesting
  • 2f96867
    • Simple change of the switch block to the switch expression + removes an unused enum value

@MihaZupan MihaZupan added this to the 11.0.0 milestone Nov 26, 2025
@MihaZupan MihaZupan self-assigned this Nov 26, 2025
@dotnet-policy-service
Copy link
Contributor

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

@MihaZupan MihaZupan marked this pull request as ready for review November 27, 2025 04:43
Copilot AI review requested due to automatic review settings November 27, 2025 04:43
Copilot finished reviewing on behalf of MihaZupan November 27, 2025 04:45
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

This PR refactors the InitializeUri method to improve readability and remove unreachable code paths without changing behavior. The changes reorganize the initialization logic to make it clearer which conditions are mutually exclusive and to eliminate dead branches.

Key changes:

  • Reorganized InitializeUri to handle errors first, then process valid absolute URIs
  • Changed InitializeUri to return UriFormatException? instead of using an out parameter
  • Converted GetException method to use a switch expression with UnreachableException for the default case

Reviewed changes

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

File Description
UriExt.cs Refactored InitializeUri method to simplify control flow, removed unreachable branches, and changed exception handling from out parameter to return value
UriEnumTypes.cs Removed unused NonEmptyHost enum value from ParsingError
Uri.cs Converted GetException from a switch statement to a switch expression and changed return type from nullable to non-nullable

@MihaZupan MihaZupan merged commit 9e9eb9b into dotnet:main Nov 28, 2025
92 of 95 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants