Fix silent shape errors by cwervo · Pull Request #272 · FolkComputer/folk

cwervo · 2026-06-18T23:17:06Z

After merging #261 we introduced an issue with draw/shapes.folk errors, for example:

Wish $this draws a circle with radius 10 filled true
#                                      ^ missing units, e.g. 10mm, 10cm, 10m

this should produce an error, but it gets swallowed by the shapes.folk file.

Before	After
No error	Now we correctly error

The fix

We change someone to wisher so we don't lose the context of the wisher program
We wrap anything in draw/shapes.folk that could throw an error in try/on error and bubble the error to the wisher.

osnr · 2026-06-19T00:41:19Z

This sort of matches what we do to report pipeline compilation errors (although that's in an imperative loop and not a When), and it should work, but isn't really the solution I was hoping for -- if possible, I think it would be better to fix this problem in general by having any error in a When block bubble up to the statement producer, although I'm not sure how hard that is

(we would probably Say an error statement per level of the 'statement trace', walking up the match/statement graph, instead of just Say for the leaf level? again, not sure if that works in practice, but something like that)

(and then we wouldn't need to insert these repetitive try-catch blocks at every point in the standard library where we want to report errors to caller)

cwervo · 2026-06-19T00:44:54Z

Gotcha, yeah, that makes sense. I went with this approach because it was the most conservative but I think it’s pretty tractable to add something in prelude.tcl that can walk the graph and issue that Say instead! Will take another swing at it.

… bubbling

This commit resolves an issue where errors from evaluated When blocks were either lost or incorrectly attributed when multiple parent statements contributed to a match. It propagates errors up through the bipartite graph to all responsible ancestor statements, while properly handling stack traces and Claim statements for error rendering. - db.c / db.h: - Update Statement and Match creation to accept and store an array of all parent statements rather than a single parent. - Add dbGetCausalTrace to walk the causal graph (BFS) and return a list of all ancestor program files. - folk.c: - Move error propagation out of Tcl and into C's runWhenBlock. - Traverse dbGetCausalTrace on JIM_ERR and dispatch SayWithSource to report errors to all responsible ancestors. - Properly format -errorinfo tuples and safely strdup the error message to prevent JimTcl memory corruption. - prelude.tcl: - Remove single-parent error bubbling logic from evaluateBlock. - Let -code error pass the error to runWhenBlock natively. - builtin-programs/errors.folk: - Update error and warning matchers to also match Claim statement outputs so bubbled errors render on the table.

cwervo · 2026-06-20T00:06:04Z

Alright, with 23be48 we now have errors that correctly propagate back to their original Folk program:

This involved making a few deeper (but relatively small!) changes:

Graph-Walking Causal Trace (db.c / db.h): Match and Statement structs now preserve an array of all parent statements instead of just a single parent. We added dbGetCausalTrace to do a BFS walk of this graph to get all the ancestors of the match.
Native Error Bubbling (folk.c): Error propagation logic is now handled in C's runWhenBlock rather than in Tcl. When JIM_ERR is thrown, we traverse the causal graph using dbGetCausalTrace and automatically dispatch SayWithSource to report the error back to every responsible ancestor program.
Fixed Memory Corruption (folk.c): Jim_EvalObjVector overwrites the interpreter's result object on evaluation. Because we're now dispatching multiple SayWithSource calls in a loop using Jim_EvalObjVector, we added a strdup to safely copy the error message and avoid rendering corrupted memory (which was showing up as "p?").
Cleaner Tcl Bubbling (prelude.tcl): We removed the older, single-parent Tcl error bubbling logic inside evaluateBlock, allowing it to cleanly throw -code error up to the C handler.
Rendering errors on the table (errors.folk): We now match against both standard error / warning statements AND their claims ... equivalents, ensuring bubbled errors always correctly draw their outlines on the table.

osnr · 2026-06-20T05:57:03Z

    Wish $p is titled $err
 }

+When /someone/ claims /p/ has error /err/ with info /info/ {


Do we need these new Whens in this file? Do they fix anything? These cases should already be handled by the non-claims When

osnr · 2026-06-20T05:57:46Z

 }

-When /someone/ wishes /p/ draws a /shape/ with /...options/ &\
+When /wisher/ wishes /p/ draws a /shape/ with /...options/ &\


Can you revert all the changes to this file? (In general, can you read the diff and remove cruft like this before asking for review?)

@cwervo mentioned that in the PR request, "We change someone to wisher so we don't lose the context of the wisher program," but I'm not entirely sure why /someone/ would be losing context?

@cwervo mentioned that in the PR request, "We change someone to wisher so we don't lose the context of the wisher program," but I'm not entirely sure why /someone/ would be losing context?

I think these changes are a relic from the original design of this PR where it just did manual try-catch in all the draw Whens and then would manually re-Say the error on $wisher.

osnr · 2026-06-20T05:59:00Z

Will test and read the new code soon, thanks!

smj-edison · 2026-06-20T07:40:23Z

+    // The program that caused the chain of matches leading to this statement.
+    char causalityFileName[100];


I'm really struggling to understand what "causality" means in this context. This is the comment that should go more in detail as to what causality means, or it should at least refer to a part of the code where causality is detailed. This would be the place to put "why" comment, not a "what" comment.

I guess my main question is why this is different than sourceFileName, or getting the filename by following to the parent?

smj-edison · 2026-06-20T07:44:08Z

    int workerThreadIndex;

+    int nParentStatements;
+    StatementRef* parentStatements;


I'm not a massive fan of bidirectional pointers, but I think in this case it makes sense, because traversal upwards would be tricky otherwise.

I was worried about the thread-safety implications of these new properties, but I think it's fine because 1. they are immutable (set at object construction when access is exclusive) and 2. they are weak refs anyway

smj-edison · 2026-06-20T07:52:56Z

+    trace[0] = '\0';
+
+    // Simple BFS queue
+    MatchRef queue[1024];


Not a massive fan of hardcoding 1024 here, since it's used in a couple different places below where it could desynchronize pretty quickly. Also could you expand the BFS acronym? I'm assuming you mean breadth first search, but it took me a bit to figure that out.

smj-edison · 2026-06-20T07:54:16Z

    }
 }
+
+// Returns a comma-separated list of unique files in the causal trace.


Is this topologically sorted? Might be worth mentioning in the comment, since otherwise someone may be unsure if it could be used for a stack trace (nice idea btw of using unique file names).

smj-edison · 2026-06-20T07:55:28Z

+            // add to trace if unique
+            if (fname && strlen(fname) > 0 && strcmp(fname, "(null)") != 0) {
+                // Check if already in trace
+                if (!strstr(trace, fname)) {


This becomes O(n^2) which is probably fine for now, but might be worth adding a TODO. Same with the strcat, since it uses a linear scan on each append.

smj-edison · 2026-06-20T07:57:43Z

                                    MatchRef parent,
                                    StatementRef* outReusedStatementRef);

+// Returns a comma-separated list of unique files in the causal trace.


Again, there's the term "causal trace", but I never really feel like it's been defined anywhere.

smj-edison · 2026-06-20T08:01:37Z

+                Jim_DictAddElement(interp, errorInfoObj,
+                                   Jim_NewStringObj(interp, "-errorinfo", -1),
+                                   errorInfoStr);
+                Jim_Obj *cmd[] = {


Can't you call the function here directly? iirc you don't need to go through the interpreter for this.

smj-edison · 2026-06-20T08:20:17Z

-                                    sourceFileName, sourceLineNumber);
+    StatementRef ref = statementNew(db, clause, keepMs, atomicallyVersion,
+                                   sourceFileName, sourceLineNumber,
+                                   parentMatch ? parentMatch->causalityFileName : sourceFileName,


So is "causality" essentially the first Wish or Claim? Won't that mean that "causality" will always just be whatever the very first claim is, i.e. the claim from the interpreter at the very beginning?

smj-edison · 2026-06-20T08:30:37Z

        pthread_mutex_unlock(&parentStatements[i]->destructorSetMutex);
+
+        // Inherit causality from the last parent (usually the triggering statement).
+        if (i == nParents - 1) {


Why is this in the for loop at all? Plus why are the multiple parent statements being tracked if you flatten out causality? I think this would work with a single parentStatementRef, instead of all the overhead for moving around the statements list. Remember, currently there's only two possible length of parentStatements: a root When, which has only one parent (the When), and when the When unifies with a statement, so the parent statements are the When and the statement it unified with.

smj-edison · 2026-06-20T08:53:13Z

        Jim_MakeErrorMessage(interp);
-        const char *errorMessage = Jim_GetString(Jim_GetResult(interp), NULL);
+        const char *errorMessageRaw = Jim_GetString(Jim_GetResult(interp), NULL);
+        char *errorMessage = strdup(errorMessageRaw ? errorMessageRaw : "Unknown error");


Jim_GetString guarantees to return a string, so this is unnecessary defensive programming.

smj-edison · 2026-06-20T08:58:13Z


-    const char* fileName = statementSourceFileName(stmt);
+    const char* fileName = statementCausalityFileName(stmt);
    int lineNumber = statementSourceLineNumber(stmt);


Don't you need to track the causality line number?

smj-edison · 2026-06-20T09:17:24Z

+                Jim_Obj *cmd[] = {
+                    Jim_NewStringObj(interp, "SayWithSource", -1),
+                    Jim_NewStringObj(interp, tok, -1), /* sourceFileName */
+                    Jim_NewIntObj(interp, 1),          /* sourceLineNumber */


Line number is erased here too.

smj-edison · 2026-06-20T09:59:05Z

Overall I think this is a really cool idea, and the implementation has some unique solutions to the problem. However, reading through this, I felt like causality is not well defined. I think I've pieced together what the code does? Essentially, causality tracks the line information of the root Say in a sequence of Says. This isn't really useful though, because the root Say starts in the interpreter. I also never saw where the new causalityFileName is used. Maybe I'm not understanding the code though.

In general, I feel like causality as conceived by this PR still has some architectural issues:

Is causality inherited via causalityFileName, or is it a breadth-first search starting from the statement? I don't see why there's both.
Why isn't the stack trace of statements reported via the SayWithSource (line 963)? The stack trace of a distant When's eval error will make no sense without the stack trace of the claims and wishes that led to it. I think it makes the most sense for a blended stack trace to exist, where you see the procedures called at the top, but also all the statements that led to it below it. Note that Tcl captures the stack trace as a list, so you should be able to blend the two. In fact, I think [errorInfo] is also implemented in Tcl in Jim's prelude, so modifying it shouldn't be too hard.
How do Hold! and Assert! play into this? They both can support statements, yet they wouldn't show up in the breadth-first walk. I'd be curious if it would be possible to capture the stack trace of where those were called from, something analogous to Zig's ConfigurableTrace (ConfigurableTrace walks the stack and saves the result into a statically sized array, it's surprisingly fast).
Maybe a bit off-topic, but could we look into using -errorcode in Tcl? It's one of Tcl's little-known features that's surprisingly powerful. Essentially, whenever you return an error, you can specify an additional error code with it. The error code is meant to be a machine-readable list of codes (such as {TCL OOM} or {AIO READFAIL}). You can match to a certain depth using try -trap {AIO} or try -trap {AIO READFAIL} depending on how granular you want the error catch to be.

I'm really excited to see where this goes!

osnr · 2026-06-20T17:04:46Z

-        /* exit(EXIT_FAILURE); */

+        char* trace = dbGetCausalTrace(db, matchRef(db, self->currentMatch));
+        char* tok = strtok(trace, ",");


Just skimming, but this use of strtok is probably not thread-safe? I also wonder if we can call SayWithSource directly instead of bouncing through the Tcl interpreter call

Or we do this whole thing in Tcl. This feels like an awkward medium

osnr · 2026-06-21T00:00:26Z

    stmt->sourceLineNumber = sourceLineNumber;

+    if (causalityFileName != NULL) {
+        snprintf(stmt->causalityFileName, sizeof(stmt->causalityFileName),


It seems like we should just excise causalityFileName from the PR, but it's also worth pointing out that this use of snprintf doesn't make sense -- the size being passed in is always going to be the size of a char pointer.

osnr · 2026-06-21T04:29:37Z

+    char causalityFileName[100];
+
+    // The match that created this statement, if any.
+    MatchRef parentMatch;


It should probably be noted here that this is purely for ancestor walk for error reporting and that it isn't used for the reactive semantics.

I also wonder how this interacts with statement reuse? A match has an immutable, canonical set of parent statements that are set at birth, but a statement can have many different (and/or successive) match parents because of deduplication. Is this the first match parent that spawned this statement? (probably fine -- that's what we do with sourceFileName and sourceLineNumber -- but should be explicitly noted)

I'd be super curious to see if we could list current ancestors using an atomic linked list or similar, so whenever you support a statement you also register where you come from. This is rough on the design side, but it would be incredible if you got a full stack trace from both C and Tcl with every statement insert.

I think I was originally doing that, but obviously the current atomic counter of supports was easier to figure out and has been sufficient for the reactive behavior. I bet we could get it to work though

(+ I was attracted to making the database graph unidirectional, which has been pretty nice until now…)

Add try/error handling to shape Whens

7d4cfb0

cwervo assigned osnr Jun 18, 2026

cwervo added 4 commits June 18, 2026 23:38

Auto-blame triggering program on When block errors

c200f4e

Add causality propagation to Statement and Match for transitive error…

7606231

… bubbling

Remove try-catch blocks from shapes.folk

daa7f32

cwervo requested review from Copilot, osnr and smj-edison and removed request for Copilot June 20, 2026 00:06

Copilot started reviewing on behalf of cwervo June 20, 2026 00:07 View session

osnr reviewed Jun 20, 2026

View reviewed changes

smj-edison reviewed Jun 20, 2026

View reviewed changes

osnr reviewed Jun 20, 2026

View reviewed changes

osnr reviewed Jun 21, 2026

View reviewed changes

		// The program that caused the chain of matches leading to this statement.
		char causalityFileName[100];

Uh oh!

Uh oh!

Conversation

cwervo commented Jun 18, 2026

The fix

Uh oh!

osnr commented Jun 19, 2026

Uh oh!

cwervo commented Jun 19, 2026

Uh oh!

cwervo commented Jun 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

osnr commented Jun 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smj-edison commented Jun 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants