[Repo Assist] docs: clarify reachability of TODO branches in JSON code generator#1716
Draft
github-actions[bot] wants to merge 2 commits intomainfrom
Draft
Conversation
Replace three misleading '//TODO: not covered in tests' comments in JsonConversionsGenerator.fs and JsonGenerator.fs with accurate explanations of WHY those branches are not exercised in practice. JsonConversionsGenerator.fs – convertJsonValue: - TypeWrapper.Option, false: reachable in principle (called when canPassAllConversionCallingTypes=false, e.g. for array element types), but currently unreachable in practice because JSON null- stripping prevents optional primitives from appearing as array elements. Document the intended semantics clearly. - TypeWrapper.Nullable, true/false: JSON inference only ever emits TypeWrapper.None or TypeWrapper.Option. These branches exist for structural completeness and potential future extensibility. JsonGenerator.fs – generateObjectMembers getter (ConvertOptionalProperty): - Requires a property whose type is IsOptional=true AND whose ConversionCallingType is JsonDocument AND which has an OptionalConverter. This combination requires InferedType.Json(optional=true), which the JSON provider does not currently produce. No behaviour change; pure documentation improvement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 This is an automated pull request from Repo Assist, an AI assistant for this repository.
Summary
Replaces three long-standing
//TODO: not covered in testscomments in the JSON code generator with accurate explanations of why each branch is not reached in practice. No behaviour changes.Motivation
These
//TODOcomments have existed since theTypeWrapper-based code-generation infrastructure was first introduced. They were correct in spirit (the branches are genuinely not exercised by the test suite) but misleading in implication — they suggest "add tests" when the actual situation is that the branches are structurally unreachable given how JSON inference works today.After tracing the full call graph, the findings are:
JsonConversionsGenerator.fs—convertJsonValueTypeWrapper.Option, falsecanPassAllConversionCallingTypes=false, e.g. for array element types), but not reachable in practice — JSON null-stripping ingenerateJsonTypestripsNulltags from collections before element types are processed, preventing optional primitives from appearing as array elements. Comment now documents the intended semantics.TypeWrapper.Nullable, trueTypeWrapper.NoneorTypeWrapper.Option(viaPrimitiveInferedValue.Create(_, bool, _)).TypeWrapper.Nullablerequires explicit"int?"schema annotations which are blocked for JSON byStructuralInference.parseTypeAndUnit. Retained for structural completeness / future extensibility.TypeWrapper.Nullable, falseJsonGenerator.fs—getterfor object membersConvertOptionalPropertyarmprop.Type.IsOptional = trueANDConversionCallingType = JsonDocumentANDOptionalConverter = Some _. This combination would needInferedType.Json(optional=true), butInferedType.Jsonis rejected bygenerateJsonTypewithfailwith "Json type not supported". Dead code in practice.Change Details
src/FSharp.Data.DesignTime/Json/JsonConversionsGenerator.fs— 3//TODO: not covered in testsreplaced with detailed explanatory commentssrc/FSharp.Data.DesignTime/Json/JsonGenerator.fs— 1//TODO: not covered in testsreplaced with detailed explanatory commentTest Status
dotnet build src/FSharp.Data.DesignTime/FSharp.Data.DesignTime.fsproj— 0 errors, 15 pre-existing warningsdotnet test tests/FSharp.Data.Core.Tests— 2896 passeddotnet test tests/FSharp.Data.DesignTime.Tests— 436 failures (pre-existing infrastructure failures identical onmainbefore this change, unrelated to the comment updates)