diff --git a/src/core/m-gc.c b/src/core/m-gc.c index 7332c77d2e..af6cc4b24a 100755 --- a/src/core/m-gc.c +++ b/src/core/m-gc.c @@ -508,30 +508,31 @@ static void Mark_Root_Series(void) assert(not IS_SER_DYNAMIC(s)); assert( not LINK(s).owner - or LINK(s).owner->header.bits & NODE_FLAG_MANAGED + or (LINK(s).owner->header.bits & NODE_FLAG_MANAGED) ); if (not (s->header.bits & NODE_FLAG_MANAGED)) { assert(not LINK(s).owner); } - else if (GET_SERIES_INFO(LINK(s).owner, INACCESSIBLE)) { - if (NOT_SERIES_FLAG(LINK(s).owner, VARLIST_FRAME_FAILED)) { - // - // Long term, it is likely that implicit managed-ness - // will allow users to leak API handles. It will - // always be more efficient to not do that, so having - // the code be strict for now is better. - // - #if !defined(NDEBUG) - printf("handle not rebReleased(), not legal ATM\n"); - #endif - panic (s); - } - - GC_Kill_Series(s); + else if ( + GET_SERIES_FLAG(LINK(s).owner, VARLIST_FRAME_FAILED) + ){ + GC_Kill_Series(s); // auto-free API handles on failure continue; } - else // note that Mark_Frame_Stack_Deep() will mark the owner + else if (not Is_Frame_On_Stack(CTX(LINK(s).owner))) { + // + // Long term, it is likely that implicit managed-ness + // will allow users to leak API handles. It will + // always be more efficient to not do that, so having + // the code be strict for now is better. + // + #if !defined(NDEBUG) + printf("handle not rebReleased(), not legal ATM\n"); + #endif + panic (s); + } + else // Note that Mark_Frame_Stack_Deep() will mark the owner s->header.bits |= NODE_FLAG_MARKED; // Note: Eval_Core() might target API cells, uses END diff --git a/src/include/datatypes/sys-context.h b/src/include/datatypes/sys-context.h index 56786fbc6a..01c93c9edc 100644 --- a/src/include/datatypes/sys-context.h +++ b/src/include/datatypes/sys-context.h @@ -189,6 +189,11 @@ static inline void INIT_CTX_KEYLIST_UNIQUE(REBCTX *c, REBARR *keylist) { #define CTX_KEYS_HEAD(c) \ SER_AT(REBVAL, SER(CTX_KEYLIST(c)), 1) // a CTX_KEY can't hold a RELVAL +inline static bool Is_Frame_On_Stack(REBCTX *c) { + assert(IS_FRAME(CTX_ARCHETYPE(c))); + return (LINK_KEYSOURCE(c)->header.bits & NODE_FLAG_CELL); +} + inline static REBFRM *CTX_FRAME_IF_ON_STACK(REBCTX *c) { REBNOD *keysource = LINK_KEYSOURCE(c); if (not (keysource->header.bits & NODE_FLAG_CELL)) diff --git a/src/include/datatypes/sys-frame.h b/src/include/datatypes/sys-frame.h index 38c9ca0473..11534869ea 100644 --- a/src/include/datatypes/sys-frame.h +++ b/src/include/datatypes/sys-frame.h @@ -363,7 +363,7 @@ inline static void UPDATE_EXPRESSION_START(REBFRM *f) { inline static void Abort_Frame(REBFRM *f) { if (f->varlist and NOT_SERIES_FLAG(f->varlist, MANAGED)) - GC_Kill_Series(SER(f->varlist)); // not alloc'd with manuals tracking + GC_Kill_Series(SER(f->varlist)); // not alloc'd with manuals tracking TRASH_POINTER_IF_DEBUG(f->varlist); // Abort_Frame() handles any work that wouldn't be done done naturally by @@ -409,7 +409,7 @@ inline static void Abort_Frame(REBFRM *f) { } } -pop:; + pop: assert(TG_Top_Frame == f); TG_Top_Frame = f->prior; @@ -702,17 +702,29 @@ inline static void Drop_Action(REBFRM *f) { } else if (GET_SERIES_FLAG(f->varlist, MANAGED)) { // - // The varlist wound up getting referenced in a cell that will outlive - // this Drop_Action(). The pointer needed to stay working up until - // now, but the args memory won't be available. But since we know - // there were outstanding references to the varlist, we need to - // convert it into a "stub" that's enough to avoid crashes. + // Varlist wound up getting referenced in a cell that will outlive + // this Drop_Action(). + // + // !!! The new concept is to let frames survive indefinitely in this + // case. This is in order to not let JavaScript have the upper hand + // in "closure"-like scenarios. See: + // + // "What Happens To Function Args/Locals When The Call Ends" + // https://forum.rebol.info/t/234 + // + // Previously this said: + // + // "The pointer needed to stay working up until now, but the args + // memory won't be available. But since we know there are outstanding + // references to the varlist, we need to convert it into a "stub" + // that's enough to avoid crashes. // // ...but we don't free the memory for the args, we just hide it from // the stub and get it ready for potential reuse by the next action // call. That's done by making an adjusted copy of the stub, which - // steals its dynamic memory (by setting the stub not HAS_DYNAMIC). + // steals its dynamic memory (by setting the stub not HAS_DYNAMIC)." // + #if 0 f->varlist = CTX_VARLIST( Steal_Context_Vars( CTX(f->varlist), @@ -721,6 +733,10 @@ inline static void Drop_Action(REBFRM *f) { ); assert(NOT_SERIES_FLAG(f->varlist, MANAGED)); INIT_LINK_KEYSOURCE(f->varlist, NOD(f)); + #endif + + INIT_LINK_KEYSOURCE(f->varlist, NOD(f->original)); + f->varlist = nullptr; } else { // We can reuse the varlist and its data allocation, which may be diff --git a/src/include/datatypes/sys-string.h b/src/include/datatypes/sys-string.h index d6cea46645..1f0eb292a1 100644 --- a/src/include/datatypes/sys-string.h +++ b/src/include/datatypes/sys-string.h @@ -560,7 +560,10 @@ inline static REBCHR(*) STR_AT(REBSTR *s, REBLEN at) { else { if (len < sizeof(REBVAL)) { if (not IS_STR_SYMBOL(s)) - assert(not bookmark); // mutations must ensure this + assert( + not bookmark // mutations must ensure this usually but... + or GET_SERIES_FLAG(s, ALWAYS_DYNAMIC) // !!! mold buffer? + ); goto scan_from_tail; // good locality, avoid bookmark logic } if (not bookmark and not IS_STR_SYMBOL(s)) { diff --git a/tests/context/bind.test.reb b/tests/context/bind.test.reb index b38a1b5b8c..4b1fc06488 100644 --- a/tests/context/bind.test.reb +++ b/tests/context/bind.test.reb @@ -48,6 +48,5 @@ [#1893 ( word: reeval func [x] ['x] 1 - e: trap [same? word bind 'x word] - e/id = 'expired-frame + same? word bind 'x word )] diff --git a/tests/context/valueq.test.reb b/tests/context/valueq.test.reb index 96cd8c7a5d..8d80eda9e3 100644 --- a/tests/context/valueq.test.reb +++ b/tests/context/valueq.test.reb @@ -1,5 +1,7 @@ ; functions/context/valueq.r (false == set? 'nonsense) (true == set? 'set?) -; #1914 ... Ren-C indefinite extent prioritizes failure if not indefinite -(error? trap [set? reeval func [x] ['x] blank]) + +[#1914 ( + set? reeval func [x] ['x] blank +)] diff --git a/tests/datatypes/function.test.reb b/tests/datatypes/function.test.reb index 5a4b41ffd9..d3f9966580 100644 --- a/tests/datatypes/function.test.reb +++ b/tests/datatypes/function.test.reb @@ -331,15 +331,24 @@ f: does (reduce [does [true]]) f )] -; no-rebind test--succeeds in R3-Alpha but fails in Ren-C. Second time f is -; called, `a` has been cleared so `a [d]` doesn't recapture the local, and -; `c` holds the `[d]` from the first call. -( - a: func [b] [a: _ c: b] - f: func [d] [a [d] do c] + +; Second time f is called, `a` has been cleared so `a [d]` doesn't recapture +; the local, and `c` holds the `[d]` from the first call. This succeeds in +; R3-Alpha for a different reason than it succeeds in Ren-C; Ren-C has +; closure semantics for functions so the c: [d] where d is 1 survives. +; R3-Alpha recycles variables based on stack searching (non-specific binding). +( + a: func [b] [ + a: _ comment "erases a so only first call saves c" + c: b + ] + f: func [d] [ + a [d] + do c + ] did all [ 1 = f 1 - error? trap [2 = f 2] + 1 = f 2 ] ) [#1528