fix(beam): make reflection metadata agree with record/union codegen - #4849
Merged
Conversation
Record construction and record reflection sanitized field names with two
different functions. `NewRecord` keys the map with `sanitizeFieldName`, which
appends a trailing `_` to lowercase-first names so `firstName` and `FirstName`
can coexist as atoms; `makePropertyInfo` derived `erl_name` with
`sanitizeErlangName`, which trims exactly that underscore. For any field whose
F# name starts lowercase, reflection read a key the constructor never wrote:
type Lower = { alpha: string; beta: int }
FSharpValue.GetRecordFields (box l) // {badkey,alpha}
FSharpValue.MakeRecord(typeof<Lower>, ...) // "succeeds", then
built.alpha // {badkey,alpha_}
PascalCase fields are unaffected, since both functions then agree — which is
why the existing reflection tests never caught it.
Derive `erl_name` with `sanitizeFieldName`, the same function codegen keys the
map with, and document it as the single source of truth for record-map keys.
Two further fixes fall out of that:
- `sanitizeFieldName` was missing the character cleanup `sanitizeErlangName`
does, so a field named `base'` produced the atom `base'_`, printed as
`'base'_'` — invalid Erlang. Factored the cleanup into `stripNonAtomChars`
and routed both sanitizers through it.
- Codegen tags a union case with its `[<CompiledName>]` verbatim, but
reflection's `erl_tag` always snake_cased the F# name, so `MakeUnion` built
a tag no pattern match looks for. Both now go through `unionCaseTagName`.
`FSharpValue.GetRecordFields` on a value whose type is not statically known
(the `obj` returned by `MakeRecord`) still cannot work — a Beam record map
carries no type tag — but it now reports `record_type_unknown_at_compile_time`
instead of an opaque `{badkey,fields}`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.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.
The bug
Record construction and record reflection sanitized field names with two different functions, and they disagree for any record field whose F# name starts with a lowercase letter.
NewRecordkeys the record map withsanitizeFieldName, which appends a trailing_to lowercase-first names sofirstNameandFirstNamecan coexist as atoms.makePropertyInfoderivederl_name— documented as "the sanitized name the field is actually keyed by in the record map" — withsanitizeErlangName, which trims exactly that underscore.The two failures point in opposite directions, which confirms the split:
MakeRecordbuilds#{alpha => ...}(reflection's name) while the compiled accessor readsalpha_(codegen's name). PascalCase fields are unaffected, since both functions then agree ondescription/count— which is why every existing reflection test, all of which use PascalCase fields, missed it.The real-world shape that hits it on every field is a remoting-style API contract, whose fields are idiomatically camelCase:
JS and Python are unaffected; only Beam.
The fix
Derive
erl_namewithsanitizeFieldName— the same function codegen keys the map with — and document it as the single source of truth for record-map keys so the two cannot drift again.Two further fixes fall out of that:
sanitizeFieldNamewas missing the character cleanup thatsanitizeErlangNamedoes. A field namedbase'produced the atombase'_, printed as'base'_'— invalid Erlang, which broke compilation of the existingPatternMatchTests. This was already latent for any record with such a field, since codegen keyed the map with it too. The cleanup is now factored intostripNonAtomCharsand both sanitizers route through it.Union
erl_tagignored[<CompiledName>]. Codegen tags a case with itsCompiledNameverbatim, but reflection always snake_cased the F# name, soMakeUnionbuilt a tag no pattern match looks for. Both now go through a sharedunionCaseTagName.Secondary
FSharpValue.GetRecordFieldson a value whose type is not statically known (theobjreturned byMakeRecord) still cannot work — a Beam record map carries no type tag, so no TypeInfo is recoverable at runtime — but it now reports{record_type_unknown_at_compile_time, FullName}instead of an opaque{badkey,fields}. Rejecting it at compile time with a proper diagnostic is still open; Beam'sReplacements.fshas no warning path today.Tests
Seven tests added to
tests/Beam/ReflectionTests.fs, covering what the PascalCase-only suite could not:GetRecordFields/PropertyInfo.GetValue/GetRecordFieldon a lowercase-first record.MakeRecordon that record, read back through the compiled accessor (r.alpha), not through reflection — this is what catches the silent-corruption direction.{ end_: int; fun_: int }), to be surecheckErlKeywordsstill composes.{ name: string; Name: string }record, pinning that the disambiguating underscore still keeps the two apart.... -> Async<'T>fields invoked through the returnedPropertyInfo— the remoting shape above.MakeUnion/GetUnionFieldson a case carrying[<CompiledName>].Verified:
dotnet test tests/Beam(ReflectionTests filter) — 78 passed on .NET, new tests included../build.sh test beam— 2658 passed, 0 failed.All changes are confined to
src/Fable.Transforms/Beam/andsrc/fable-library-beam/; no shared files touched, so no other target is affected.🤖 Generated with Claude Code