feat: add generated request building support for path parameters in the URL.

[calebkiage]

What kind of change does this PR introduce?

Adds generated request building support for path parameters provided in the URL.

What is the new behavior?

If a URL has path parameters defined in the URL and the method signature has parameters matching the path parameter, then the source generator will create a method that uses source generation.

The method signature below will now use a generated method body.

interface IProfileClient {
    [Get("/users/{userId}"]
    Task<User> GetUser(string userId);
}

What is the current behavior?

The old behavior used reflection for any methods that have parameters in the path.

What might this PR break?

None

Checklist

• I have read the Contribute guide
• Tests have been added or updated (for bug fixes / features)
• Docs have been added or updated (for bug fixes / features)
• Changes target the main branch
• PR title follows Conventional Commits

Additional information

[glennawatson]

Thanks for the draft. Know it's WIP - a few notes that should help when you pick it back up.

Runtime bug in GeneratedRequestRunner.BuildRequestPath:

var isPathParam = index > 0 && toReplace[index - 1] == '/';

toReplace[index - 1] indexes the literal "{key}" instead of path; should be path[index - 1]. As written, /users/{user} throws IndexOutOfRangeException and /{id} leaves the placeholder unreplaced. One-char fix.

Test gap: the new tests only check generated text + CompilesWithoutErrors; nothing executes a path-param request. The one emit-load-invoke test uses [Get("/users")] (no param). An emit-load-invoke case asserting the final RequestUri (single, multiple, non-string, null/empty) would catch the bug above - most valuable addition here.

Smaller, for later:

• Empty value drops the whole segment (/users/{user} -> /users); reflection substitutes ?? string.Empty -> /users/. Worth aligning so inline and reflective URIs match.
• replacementBlockRegex adds a per-match alloc into the parse hot path stage 1 de-LINQ'd, and the camelCase static name likely trips the analyzers; a manual scan avoids both and drops the duplicate brace logic.
• Confirm params ReadOnlySpan<(string, string?)> (C# 13) compiles on the net4x TFMs - no CI ran on the branch.

[calebkiage]

@glennawatson, good catch on the bug. Was working on this late.

Test gap: the new tests only check generated text + CompilesWithoutErrors; nothing executes a path-param request.

It looks like there are already tests that check for parameter substitution. They were failing and I've fixed most of them.

Empty value drops the whole segment (/users/{user} -> /users); reflection substitutes ?? string.Empty -> /users/. Worth aligning so inline and reflective URIs match.

So if a path like /users/{id}/orders gets a null value then the new URL will be /users//orders?

---

How should generated code behave when IUrlParameterFormatter has been set? Currently, the generated code doesn't apply any formatting to the values.

[glennawatson]

So if a path like /users/{id}/orders gets a null value then the new URL will be /users//orders?

Yep, agreed -- and I think that's the behavior we actually want. It matches what reflection already does (Format(...) ?? string.Empty) and it's RFC 6570 simple expansion (an empty var expands to ""), so inline and reflective URIs stay identical. I'd rather keep the // than have the generator special-case it and drift from reflection. If we ever want a null path arg to fail fast, that's a separate change we'd make in both paths.

How should generated code behave when IUrlParameterFormatter has been set? Currently, the generated code doesn't apply any formatting to the values.

This is the part I care more about -- it's a real parity gap vs reflection. Let's get the generated code calling into it: route each path value through settings.UrlParameterFormatter.Format(value, attributeProvider, type) instead of .ToString(). The only snag is the ICustomAttributeProvider arg -- we don't want to reflect ParameterInfo at runtime under AOT. Could we pass the statically-known typeof(TParam) plus a generator-emitted (or empty) ICustomAttributeProvider, and emit a real per-parameter provider later if a formatter needs the attributes? That honors custom/type formatters in the common case and keeps us off reflection.

[calebkiage]

I have the first iteration of UrlParameterFormatter support. Can you take a look?

[glennawatson]

Really nice progress, Caleb. Routing path values through UrlParameterFormatter is the right move for reflection parity, and the span-based scan in BuildRequestPath is clean. I traced single-param, multi-param, leading-placeholder, and trailing-segment cases and they all came out correct.

I left four inline suggestions below. Two are correctness blockers (the GetCustomAttributes throw and the AliasAs name check, both verified against a real build / Roslyn run) and two are small cleanups (SpecialType over name matching, and not emitting provider fields for non-path params). Each has a one-click suggestion attached.

A few things that are not inline:

Test coverage. The new tests assert generated text plus CompilesWithoutErrors, which is good for shape, but nothing executes a path-param request and checks the final RequestUri. An emit-load-invoke test (single param, multiple params, non-string, null/empty, and importantly a path param carrying a non-Query attribute) would have caught the GetCustomAttributes crash immediately. Highest-value addition before merge.

Type coverage (worth pulling in). Guid, DateTime/DateTimeOffset, decimal, double/float, and enums currently fall back to reflection. Guid and DateTime are probably the two most common route types, so accelerating them is a real win rather than an edge case. The SpecialType rewrite in the IsSimpleType suggestion makes the numeric ones (System_Decimal, System_Double, System_Single) a one-line add; Guid/DateTime/enum need a value-formatting parity check but are straightforward. Fine as a fast-follow if you would rather keep this PR tight.

Dotted and round-trip placeholders ({request.Prop}, {**param}). Reflection supports these and your code correctly falls back today, so there is no regression. The ask is just to add a test that pins that fallback so it cannot silently break later. If you decide to extend coverage to them, @TimothyMakkison models them in #2182 as a small RouteFragmentModel discriminated union (constant / standard-param / object-access / round-trip), which is a good reference.

Perf. We do care about allocations on this path, so this one is more than a nicety. BuildRequestPath re-parses the template and allocates a Dictionary plus a 1024-char pooled buffer on every call. For the common 1-2 param case, a linear scan over the tuple array (drop the dictionary) and a smaller stackalloc buffer would be allocation-free. The dictionary only pays off if the same placeholder repeats, which is rare. Bigger picture: #2182 sidesteps the runtime re-parse entirely by emitting straight-line ValueStringBuilder.Append calls at compile time. Not required for this PR, but worth seeing as a direction since perf is a focus here.

Heads-up on overlap: @TimothyMakkison is building the same feature in #2182. Rather than have you both work it in parallel, the plan is to land this PR as the base and pull in a few of his ideas as we go (all optional, your call on scope). Thanks for jumping on this, it is a useful chunk of the AOT story.

[calebkiage]

Thanks for the review. I've addressed most of the changes and have also added some tests. I will update the PR with more tests later.

On the suggestion for inline path building, is it not better to share the logic than generate it on every request method? It would reduce the amount of generated code in the end. If I build the path inline, there will be a lot of duplicated code for parameter building. If perf is an issue, I can optimize it further by passing in the template and span ranges to replace instead of the parameter name. i.e. BuildRequestPath("/user/{id}", allowUnmatchedParameter: false, ((6, 10), "234")). What do you think of that idea over using generated path building?

[glennawatson]

We can run some benchmarks and see either way. I'm happy whit the approach of using shared approach as long as our reflection costs go down to close as 0 in the known cases as possible.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
90.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[glennawatson]

btw @calebkiage if it helps @TimothyMakkison is on Slack if you want to talk to him about his approach. Had a bit of a chat to him on there and he agreed taking your PR was best, since you put a bit of effort in before. But if you want you can swap ideas between each other there. https://reactiveui.net/Slack/

Just a moment...

[calebkiage]

@glennawatson, I can't join that workspace. It says my account is deactivated.

[glennawatson]

@glennawatson, I can't join that workspace. It says my account is deactivated.

Ah ok, we can use comments then that's fine.

[calebkiage]

@glennawatson, I think I broke the incremental compilation tests on the latest commit and I'm not sure where the problem is. Could you take a look?

[glennawatson]

Pushed a commit to get the build and tests green.

• Migrated the new path-parameter RestService tests to the Refit.Testing StubHttp API (dropped MockHttpMessageHandler, which is no longer referenced)
• Added a missing await and fixed the analyzer errors in the new tests; split the large runner test into a partial file
• Reworked how the generator model stores parameter locations and attributes

Why I moved it off ImmutableArray and the AttributeData field for the incremental engine:

• The incremental generator caches one model per method and only re-runs when that model changes by value equality
• ImmutableArray compares by its backing-array reference, so a fresh parse never matched the cached model and it regenerated on every edit (this is what the 3 Incremental tests were catching)
• AttributeData is worse: it never compares equal across compilations, and holding it roots the whole Compilation in the generator cache
• So locations now use ImmutableEquatableArray, and attributes are flattened into a small value record at parse time - no Roslyn symbols in the cache

Generated output is byte-identical (Verify snapshots unchanged), and all tests pass locally.

[glennawatson]

Nice work - CI is green now (build and all tests pass across every TFM). Below is a quick pass on what is likely still needed before the reflection-free path can be called "done" - the main one is URL-encoding parity, the rest are coverage/robustness follow-ups. Specifics are in the inline comments.

• Encoding (blocker): the generated path substitutes formatted values verbatim, while the reflection path URL-encodes every value, so the two pipelines produce different (and unescaped) URLs for the same input - a correctness and injection risk.
• Coverage: only route-template placeholders ({id}, and ?q={q}) generate inline; plain auto-appended query params (Task Get(string q) -> ?q=...), [Query] and [AliasAs] still fall back to reflection, so the generated-only/AOT path still throws for typical query APIs.
• Robustness: BuildRequestPath's trailing unmatched-parameter scan has a couple of edge cases (inline).
• Note (not a regression): value formatting still goes through IUrlParameterFormatter, which uses reflection for enums/TypeConverter - fine for now, but it means "reflection-free" holds at the request-shaping layer, not value formatting.

[calebkiage]

value formatting still goes through IUrlParameterFormatter, which uses reflection for enums/TypeConverter

Yeah. I'll need some time to think about how to properly handle this situation.

[glennawatson]

value formatting still goes through IUrlParameterFormatter, which uses reflection for enums/TypeConverter

Yeah. I'll need some time to think about how to properly handle this situation.

Let's just handle this in the next PR from either yourself or @TimothyMakkison

[glennawatson]

Merged this one in, we can now tackle the rest as smaller PRs.