Cover the remaining dynamic call targets for unhandled errors - #17
Merged
Conversation
The spec file landed with only two of the dynamic targets a Starlark program can build, and the Go test exercised only a variable bound to an error-returning builtin. The target most embedders actually expose, an attribute of a module-like value (fs.read_all), went untested even though it is the shape that motivated the runtime check. Add the missing targets: a parameter and the result of another call on the Starlark side, and on the Go side an attribute of a struct plus a call at module top level, where the pending error is dropped by the module frame's own return rather than a function's.
The attribute target was covered only by a Go test, where the callee is an error-returning builtin. Behind a struct field the callee is instead an ordinary ! function, so the pending error reaches the caller's frame by a different route, and no .star test took that route. The spec suite cannot express this: its predeclared vocabulary is only assert, trap, matches and freeze, with no way to build an attribute -bearing value. The package testdata environment predeclares struct, so the case belongs here.
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.
What does this PR do?
Test-only follow-up to the runtime check for unhandled errors from
dynamically-dispatched
!calls.The spec file that landed with that check covers only two of the dynamic
targets a Starlark program can build, and the Go test covers only a variable
bound to an error-returning builtin. This adds the missing ones:
spec/optional/error_handling/dynamic_calls.star— a parameter(
def f(g): g()) and the result of another call (get_handler()()),and a header comment that lists them.
starlark/eval_test.go— an attribute of a module-like value(
fs.read_all(...), viastarlarkstruct), and a bare call at module toplevel, where the pending error is dropped by the module frame's own return
rather than a function's.
starlark/testdata/dynamic_calls.star— the attribute target exercised fromStarlark, where the callee behind the struct field is an ordinary
!function rather than a builtin, so the pending error reaches the caller's
frame by the other route. The spec suite cannot express this (its predeclared
vocabulary is
assert,trap,matches,freeze, with no way to build anattribute-bearing value); the package testdata environment predeclares
struct, so it belongs there.Motivation
fs.read_all("//path/to/missing.yml")— an attribute of an embedder module —is the shape that motivated the runtime check in the first place, and it had no
test. An attribute target is never statically resolvable, so the resolver
always defers to runtime; it is the case the silent drop hit hardest, and the
one most likely to regress unnoticed.
A parameter and a call result are the other two targets expressible in Starlark
alone. Per
spec/harness.md, the attribute case cannot live inspec/, whichis why the coverage is split across the three files above.
Additional notes
No production code changes.
How to test the change?
Verified in both directions:
d44dbe2(check present)1e50f97(before the check)go test ./...spec/.../dynamic_calls.starassert.failscases failstarlark/testdata/dynamic_calls.starassert.failscases failTestUnhandledDynamicErrorReturningCallFailsBefore the check, both new Go cases return
Noneand no error at all:globals = {result: None, spec: None}.PR stack
master