fix(beam): make Assert.AreEqual/NotEqual raise, and stop emitting reflection calls for erased types#4775
Merged
Merged
Conversation
…lection calls for erased types
Calling Fable.Core.Testing.Assert.AreEqual/NotEqual directly lowered to a
bare equality expression, so the resulting boolean was computed in
statement position and discarded: the assertion could never fail. The
three-argument overload (custom message) was not supported at all and
failed to compile. Lower them to throwing library calls instead,
mirroring JS and Python.
This does not affect tests/Beam. Fable2Beam intercepts call sites whose
import selector is `Testing_equal`/`assertEqual` and inlines a raising
`case ... erlang:error({assert_equal, Expected, Actual})`, and the
suite's own `equal` in tests/Beam/Util.fs compiles to that selector, so
those assertions have always fired. The bug hits code that calls Assert
directly, such as a downstream test framework.
The new fable_utils:assert_equal/assert_not_equal raise a map carrying
`message` plus the two operands. Fable2Beam already binds a raised map
directly as the exception and reads its `message` key, so `e.Message`
works in `try ... with` while a runner can still pattern-match
`actual`/`expected` off the map, as JS does.
Reflection over an erased ([<Erase>], [<StringEnum>],
[<TypeScriptTaggedUnion>]) or global/imported entity emitted a call to a
`<entity>_reflection/0` function that is never declared, since no
declaration is emitted for such an entity. A record with a field of an
erased type therefore failed to compile when the erased type was in the
same file, and crashed at run time with `undef` when it was in another
one (Erlang does not resolve remote calls at compile time). This is a
regression from b15b280 (#4766), first shipped in 5.8.1, and it blocks
compilation of any project using [<Erase>] types in records. Report the
bare type info for those entities instead. The guard deliberately does
not include isReplacementCandidate: that would short-circuit Result and
Choice into a type info with no cases, making reflection report them as
non-unions.
Both failure modes are covered by regression tests, each verified to fail
without its fix.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
8b5010f to
b5843ab
Compare
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.
Two independent Beam-target bugs, both found while upgrading Fable.Beam from 5.6.0 to 5.8.1.
1.
Assert.AreEqual/Assert.NotEqualnever raised when called directlyBeam/Replacements.fslowered them to a bare equality expression. The resulting boolean sits in statement position, so it is computed and discarded — the assertion could never fail. The three-argument overload (custom message) was not supported at all:Assert.AreEqual(a, b, "msg")was a hard compile error (Fable.Core.Testing.Assert.AreEqual is not supported, try updating fable tool).They now lower to throwing library calls, mirroring JS (
Replacements.fs) and Python.Scope: this does not mean the Beam test suite was unasserted.
Fable2Beam.fs:2261intercepts call sites whose import selector isTesting_equal/assertEqualand inlines a raising assertion, with no runtime dependency:tests/Beam/Util.fs'sequalcompiles to that selector, so the suite's assertions have always fired (verified by corrupting an expected value under the pre-fix compiler and watching it fail). The bug bites code that callsFable.Core.Testing.Assertdirectly, which is what a downstream test framework does — that's how it was found.The new
fable_utils:assert_equal/2,3andassert_not_equal/2,3raise a map carryingmessageplus the two operands.Fable2Beamalready binds a raised map directly as the exception and reads itsmessagekey, soe.Messagereads cleanly intry ... with, while a runner can still pattern-matchactual/expectedoff the map. This mirrors JS, which throws anExceptionwith the message and attachesactual/expectedas properties.Note this leaves Beam with two assertion shapes: the inlined
{assert_equal, Expected, Actual}tuple atTesting_equalcall sites, and the new#{message, actual, expected}map from the library. Consolidating them is worth a follow-up, but is out of scope here.2. Erased types emitted dangling
<entity>_reflection()calls (regression, blocks compilation)Regression from b15b280 ("fix(beam): make F# reflection work on the Beam target", #4766), first shipped in 5.8.1.
That change made a reference to a record/union emit a call to the entity's generated
<entity>_reflection/0. But an erased ([<Erase>],[<StringEnum>],[<TypeScriptTaggedUnion>]) or global/imported entity never gets a declaration emitted, so there is no reflection function to call:Two failure modes, the second nastier:
function shutdown_reflection/0 undefined.undef.This blocks compilation of any project using
[<Erase>]types in records — an idiomatic pattern for zero-cost opaque Erlang types.Fixed by reporting the bare type info for entities that produce no declaration, which is what Python already does. The guard is deliberately kept to exactly the two "no declaration is emitted" predicates: including
isReplacementCandidate(as Python's does) short-circuitsResultandChoiceinto a type info with nocasesentry, so reflection reports them as non-unions. They have noSourcePathand so never reached the call branch anyway.Tests
tests/Beam/AssertTests.fs(new) asserts that a mismatchingAssert.AreEqual/NotEqualthrows and thate.Messageis the expected string. It callsAssertdirectly rather than through the suite'sequal, which is the path that was broken; anything that must fail the test fails it withfailwith.ReflectionTests.fs/Misc/Util2.fscover reflection over a record with an erased field in both the local and the cross-module position — only the local one is caught at compile time, so the remote variant needs its own run-time coverage.ReflectionTests.fsalready coversResult/Choice, which pins theisReplacementCandidatetrap above.Each new test was verified to fail without its corresponding fix. Full suite: 2586 passed, 0 failed.
🤖 Generated with Claude Code