Repository navigation
Ship a Roslyn analyzer with the package #17
Description
Activity
Relationship to #19: the named-arguments rule and scalar overloads solve the same bug differently
The headline rule here exists because constructing
*RequestDatapositionally silently rebinds when a protocol refresh inserts a field, which is what brokeSetSceneItemEnabledwhen 5.7 addedcanvasUuid. The scalar overloads researched in #19 attack the same bug from the other end: ifclient.Inputs.SetInputMuteAsync("Mic", true, ct)exists, the record is never constructed at the call site, so there is nothing for the rule to police.They overlap on 87 of 147 requests, which is what the overloads could cover. So the two are alternatives over most of the surface, and complements over the rest:
analyzer rule scalar overloads Coverage every request, including the 9 taking an Objectsettings payload87 requests, none of the ObjectonesWhen it helps at compile time, in consumer code too removes the hazard rather than reporting it Cost a shipped analyzer package and a suppression story a large generated surface, and one collision to resolve Fails how a squiggle cannot fail: there is no positional record left Opportunity. Landing the overloads first shrinks this rule to the 9 settings requests plus anyone who still builds records by hand, which may not justify shipping an analyzer for it alone. The other two rules proposed here,
Sleepoutside a batch and typed references underParallel, are untouched by that and keep their value either way.Cost of doing this first. An analyzer that requires named arguments everywhere, followed later by overloads that make the constructor unnecessary, leaves consumers who already adopted named arguments with nothing to change but a rule that no longer fires on their code. Harmless, but wasted.
Worth deciding the order deliberately rather than by whichever gets picked up first. My read: #19's field partition is the prerequisite for both, since it is what tells the generator which parameters an overload takes, and it is also what an analyzer needs to know a request's identity fields.
Also relevant to the
Parallelrule: reading a mispaired payload is no longer silent as of the payload shape check, so that rule is now a compile-time nicety over a runtime error rather than over silent corruption. See #16.
Nice to have, not an immediate target.
Ship analyzers alongside the library the way EF Core, ASP.NET and the BCL do, so mistakes are caught at compile time in consumer code rather than at runtime against a live OBS.
Rules worth having
Positional construction of generated request records. This caused a real bug.
SetSceneItemEnabledAsyncpassedsceneNamepositionally, and when the 5.7 protocol refresh insertedcanvasUuidinto the generated constructor the argument silently bound to the wrong parameter, so the scene was never sent and OBS answered with code 300. Parameter order follows the protocol definition and will shift again on the next refresh. An analyzer that requires named arguments for*RequestDataconstructors would have caught it at compile time, in every consumer's code as well as ours.Typed batch references with parallel execution.
BatchResults.Getrefuses at runtime when a batch ran withRequestBatchExecutionType.Parallel, because OBS mislabels those results (#16). Where the execution type is a literal at the call site, that could be a squiggle instead.Requests that are only valid inside a batch.
Sleepoutside a batch, or in a parallel batch, fails at runtime today.Notes
Analyzers ship in
analyzers/dotnet/csinside the package. Each rule needs aDiagnosticIdso consumers can suppress individually. Worth a code fix for the named-arguments rule, since that one is mechanical.