From e139967d9ab63428c9a6659b29d9502b10071daf Mon Sep 17 00:00:00 2001 From: Brian Dickens Date: Mon, 9 Oct 2023 02:20:15 -0400 Subject: [PATCH] Simplify refinement and return typechecking When refinements were adapted to be special cases of paths, and characters as special cases of issues, this introduced the need to be able to check not just a fundamental type but a "type constraint". This was done very hackily for just those examples initially, but the system is now migrating to where arbitrary functions can be used in type blocks as constraints. The messy evolution hadn't really been cleaned up (including a detour into the type constraint of tuples with leading blanks as being "predicates"). There was also separate type checking for refinements which hadn't been folded in fully when refinements became "their own arguments", that had some code in specialization that was more complex than it needed to be. This takes a step toward a bit more clarity, deleting unneeded bits and fusing the refinement type checking with the parameter checking. Also separates out the return type checking into its own function for clarity. --- extensions/javascript/mod-javascript.c | 4 +- src/core/c-error.c | 9 ++- src/core/d-eval.c | 6 +- src/core/evaluator/c-action.c | 24 +----- src/core/functionals/c-reframer.c | 11 ++- src/core/functionals/c-specialize.c | 62 +++------------ src/core/functionals/c-typechecker.c | 2 +- src/core/functionals/n-function.c | 7 +- src/core/t-varargs.c | 2 +- src/include/datatypes/sys-frame.h | 39 ++++++---- src/include/datatypes/sys-sequence.h | 15 ---- src/include/datatypes/sys-typeset.h | 100 +++++-------------------- src/include/sys-eval.h | 2 +- 13 files changed, 80 insertions(+), 203 deletions(-) diff --git a/extensions/javascript/mod-javascript.c b/extensions/javascript/mod-javascript.c index 94556aa267..8445331b2e 100644 --- a/extensions/javascript/mod-javascript.c +++ b/extensions/javascript/mod-javascript.c @@ -754,7 +754,9 @@ Bounce JavaScript_Dispatcher(Frame(*) frame_) } handle_resolved: { //////////////////////////////////////////////////////// - FAIL_IF_BAD_RETURN_TYPE(f); + if (not Typecheck_Return(f, param, OUT)) + fail (Error_Bad_Return_Type(f, OUT)); + return OUT; } handle_rejected: { //////////////////////////////////////////////////////// diff --git a/src/core/c-error.c b/src/core/c-error.c index b1cad6e744..92f1637b5c 100644 --- a/src/core/c-error.c +++ b/src/core/c-error.c @@ -1120,7 +1120,7 @@ Context(*) Error_Unexpected_Type(enum Reb_Kind expected, enum Reb_Kind actual) // a type different than the arg given (which had `arg_type`) // Context(*) Error_Arg_Type( - Frame(*) f, + option(Frame(*)) f, const REBKEY *key, const REBPAR *param, const REBVAL *arg @@ -1129,7 +1129,10 @@ Context(*) Error_Arg_Type( Init_Word(param_word, KEY_SYMBOL(key)); DECLARE_LOCAL (label); - Get_Frame_Label_Or_Nulled(label, f); + if (f) + Get_Frame_Label_Or_Nulled(label, unwrap(f)); + else + Init_Nulled(label); DECLARE_LOCAL (spec); option(Array(const*)) param_array = VAL_PARAMETER_ARRAY(param); @@ -1138,7 +1141,7 @@ Context(*) Error_Arg_Type( else Init_Block(spec, EMPTY_ARRAY); - if (FRM_PHASE(f) != f->u.action.original) { + if (f and FRM_PHASE(f) != unwrap(f)->u.action.original) { // // When RESKIN has been used, or if an ADAPT messes up a type and // it isn't allowed by an inner phase, then it causes an error. But diff --git a/src/core/d-eval.c b/src/core/d-eval.c index 6e8721493e..6d1de58b06 100644 --- a/src/core/d-eval.c +++ b/src/core/d-eval.c @@ -237,11 +237,7 @@ void Do_After_Action_Checks_Debug(Frame(*) f) { Action(*) phase = FRM_PHASE(f); if (ACT_HAS_RETURN(phase)) { - const REBKEY *key = ACT_KEYS_HEAD(phase); - const REBPAR *param = ACT_PARAMS_HEAD(phase); - assert(KEY_SYM(key) == SYM_RETURN); - - if (not Typecheck_Including_Constraints(param, f->out)) { + if (not Typecheck_Return(f, f->out)) { assert(!"Native code violated return type contract!\n"); panic (Error_Bad_Return_Type(f, f->out)); } diff --git a/src/core/evaluator/c-action.c b/src/core/evaluator/c-action.c index c7343d3aa8..dae247d1e4 100644 --- a/src/core/evaluator/c-action.c +++ b/src/core/evaluator/c-action.c @@ -156,7 +156,7 @@ Bounce Proxy_Multi_Returns_Core(Frame(*) f, Value(*) v) if (VAL_PARAM_CLASS(PARAM) != PARAM_CLASS_OUTPUT) continue; - if (not TYPE_CHECK(PARAM, ARG)) + if (not Typecheck_Parameter(PARAM, ARG)) fail (Error_Arg_Type(f, KEY, PARAM, ARG)); Meta_Quotify(Copy_Cell(PUSH(), ARG)); @@ -903,35 +903,15 @@ Bounce Action_Executor(Frame(*) f) continue; } - if ( - GET_PARAM_FLAG(PARAM, REFINEMENT) - or GET_PARAM_FLAG(PARAM, SKIPPABLE) - ){ - Typecheck_Refinement(KEY, PARAM, ARG); // extra check of # or NULL - continue; - } - - if (PARAM_CLASS_META == VAL_PARAM_CLASS(PARAM)) { - // - // !!! Now that everything is isotopic, there needs to be a new - // policy on checking here. - // - } - if (GET_PARAM_FLAG(PARAM, CONST)) Set_Cell_Flag(ARG, CONST); // mutability override? see [5] - if (GET_PARAM_FLAG(PARAM, REFINEMENT)) { - Typecheck_Refinement(KEY, PARAM, ARG); - continue; // !!! Review when # is used here - } - if (KEY_SYM(KEY) == SYM_RETURN) continue; // !!! let whatever go for now typecheck_again: - if (not Typecheck_Including_Constraints(PARAM, ARG)) { + if (not Typecheck_Parameter(PARAM, ARG)) { if (Is_Activation(ARG)) { Deactivate_If_Activation(ARG); goto typecheck_again; diff --git a/src/core/functionals/c-reframer.c b/src/core/functionals/c-reframer.c index 00ad3cf4c6..544197e58f 100644 --- a/src/core/functionals/c-reframer.c +++ b/src/core/functionals/c-reframer.c @@ -399,7 +399,14 @@ DECLARE_NATIVE(reframer_p) // Make sure the parameter is able to accept FRAME! arguments (the type // checking will ultimately use the same slot we overwrite here!) // -/* if (not TYPE_CHECK(param, REB_FRAME)) { + // !!! This checks to see if it accepts *an* instance of a frame, but it's + // not narrow enough because there might be some additional check on the + // properties of the frame. This is a limit of the type constraints + // needing an instance of the type to check. It may suggest that we + // shouldn't do this at all, and just let it fail when called. :-/ + // + Copy_Cell(SPARE, FRAME->rootvar); + if (not Typecheck_Parameter(param, SPARE)) { DECLARE_LOCAL (label_word); if (label) Init_Word(label_word, unwrap(label)); @@ -415,7 +422,7 @@ DECLARE_NATIVE(reframer_p) param_word ); goto cleanup_binder; - } */ + } } cleanup_binder: { diff --git a/src/core/functionals/c-specialize.c b/src/core/functionals/c-specialize.c index 2a53589616..f3b9f8973a 100644 --- a/src/core/functionals/c-specialize.c +++ b/src/core/functionals/c-specialize.c @@ -322,34 +322,16 @@ bool Specialize_Action_Throws( if (Is_Specialized(param)) continue; - if (GET_PARAM_FLAG(param, REFINEMENT)) { - if (Is_None(arg)) { - // - // Undefined refinements not explicitly marked hidden are - // still candidates for usage at the callsite. - - goto unspecialized_arg; // ran out...no pre-empt needed - } - - Typecheck_Refinement(key, param, arg); - goto specialized_arg_no_typecheck; - } - - // It's an argument, either a normal one or a refinement arg. - - if (Is_None(arg)) - goto unspecialized_arg; - - goto specialized_arg_with_check; - - unspecialized_arg: - - assert(Is_None(arg)); assert(IS_PARAMETER(param)); - Copy_Cell(arg, param); - continue; - specialized_arg_with_check: + // You can't specialize with ~ isotopes ("none"), these indicate + // unspecialized arguments. Only ^META parameters can take meta-none + // (e.g. a plain quasi-void, or ~) + // + if (Is_None(arg)) { // unspecialized argument + Copy_Cell(arg, param); + continue; + } // !!! If argument was previously specialized, should have been type // checked already... don't type check again (?) @@ -357,26 +339,8 @@ bool Specialize_Action_Throws( if (GET_PARAM_FLAG(param, VARIADIC)) fail ("Cannot currently SPECIALIZE variadic arguments."); - if (not Typecheck_Including_Constraints(param, arg)) - fail (arg); // !!! merge w/Error_Invalid_Arg() - - continue; - - specialized_arg_no_typecheck: - - // !!! Technically speaking, if you are trying to implement the ^META - // parameter convention, SPECIALIZE does it too late here. The intent - // of ~_~ isotope vs. NULL is lost...at least in theory, because - // variables don't store the null isotope state. The "core" way to - // do this is MAKE FRAME! (specialize could be written in usermode - // with that). But as a higher-level tool, specialize can make the - // tradeoff to make it easy by doing the meta-quoting for you...which - // also means it can keep working if the parameter convention changes. - // - if (VAL_PARAM_CLASS(param) == PARAM_CLASS_META) - Meta_Quotify(arg); - - continue; + if (not Typecheck_Parameter(param, arg)) + fail (Error_Arg_Type(nullptr, key, param, arg)); } // Everything should have balanced out for a valid specialization. @@ -752,10 +716,8 @@ Action(*) Alloc_Action_From_Exemplar( continue; } - if (GET_PARAM_FLAG(param, REFINEMENT)) - Typecheck_Refinement(key, param, arg); - else - Typecheck_Including_Constraints(param, arg); + if (not Typecheck_Parameter(param, arg)) + fail (Error_Arg_Type(nullptr, key, param, arg)); } // This code parallels Specialize_Action_Throws(), see comments there diff --git a/src/core/functionals/c-typechecker.c b/src/core/functionals/c-typechecker.c index ab9b31c3ab..1bb7c916ff 100644 --- a/src/core/functionals/c-typechecker.c +++ b/src/core/functionals/c-typechecker.c @@ -318,7 +318,7 @@ bool Typecheck_Value( if (VAL_PARAM_CLASS(param) == PARAM_CLASS_META) Meta_Quotify(arg); - if (not TYPE_CHECK(param, arg)) { + if (not Typecheck_Parameter(param, arg)) { Drop_Action(f); if (match_all) return false; diff --git a/src/core/functionals/n-function.c b/src/core/functionals/n-function.c index 029b40fe16..517a968ba5 100644 --- a/src/core/functionals/n-function.c +++ b/src/core/functionals/n-function.c @@ -620,12 +620,9 @@ DECLARE_NATIVE(definitional_return) // take [ any-value!] as its argument, and then report the error // itself...implicating the frame (in a way parallel to this native). // - if (GET_PARAM_FLAG(param, RETURN_NONE) and not Is_None(atom)) - fail ("If RETURN: is in a function spec, RETURN NONE only"); - - if (not TYPE_CHECK(param, atom)) { + if (not Typecheck_Return(target_frame, atom)) { Decay_If_Unstable(atom); - if (not TYPE_CHECK(param, atom)) + if (not Typecheck_Return(target_frame, atom)) fail (Error_Bad_Return_Type(target_frame, atom)); } diff --git a/src/core/t-varargs.c b/src/core/t-varargs.c index a6b494a4f4..c40c06138e 100644 --- a/src/core/t-varargs.c +++ b/src/core/t-varargs.c @@ -336,7 +336,7 @@ bool Do_Vararg_Op_Maybe_End_Throws_Core( Decay_If_Unstable(out); if (param) { - if (not TYPE_CHECK(param, out)) { + if (not Typecheck_Parameter(param, out)) { // // !!! Array-based varargs only store the parameter list they are // stamped with, not the frame. This is because storing non-reified diff --git a/src/include/datatypes/sys-frame.h b/src/include/datatypes/sys-frame.h index eadfaa0fce..c99c9ad44f 100644 --- a/src/include/datatypes/sys-frame.h +++ b/src/include/datatypes/sys-frame.h @@ -632,22 +632,6 @@ inline static REBVAL *D_ARG_Core(Frame(*) f, REBLEN n) { // 1 for first arg D_ARG_Core(frame_, (n)) -// Shared code for type checking the return result. It's used by the -// Returner_Dispatcher(), but custom dispatchers use it to (e.g. JS-NATIVE) -// -inline static void FAIL_IF_BAD_RETURN_TYPE(Frame(*) f) { - Action(*) phase = FRM_PHASE(f); - const REBPAR *param = ACT_PARAMS_HEAD(phase); - assert(KEY_SYM(ACT_KEYS_HEAD(phase)) == SYM_RETURN); - - // Typeset bits for locals in frames are usually ignored, but the RETURN: - // local uses them for the return types of a function. - // - if (not Typecheck_Including_Constraints(param, f->out)) - fail (Error_Bad_Return_Type(f, f->out)); -} - - inline static bool Eval_Value_Core_Throws( REBVAL *out, Flags flags, @@ -959,3 +943,26 @@ inline static Bounce Continue_Subframe_Helper( #define DELEGATE_SUBFRAME(sub) ( \ Continue_Subframe_Helper(frame_, false, (sub)), \ BOUNCE_DELEGATE) + + +inline static bool Typecheck_Return( + Frame(*) f, + Value(*) atom // Typecheck_Parameter needs mutability (unused for returns) +){ + if (Is_Pack(atom)) + return true; // For now, assume multi-return typechecked it + if (Is_Raised(atom)) + return true; // For now, all functions return definitional errors + + // Typeset bits for locals in frames are usually ignored, but the RETURN: + // local uses them for the return types of a function. + // + Action(*) phase = FRM_PHASE(f); + const REBPAR *param = ACT_PARAMS_HEAD(phase); + assert(KEY_SYM(ACT_KEYS_HEAD(phase)) == SYM_RETURN); + + if (GET_PARAM_FLAG(param, RETURN_NONE) and not Is_None(atom)) + fail ("If RETURN: is in a function spec, RETURN NONE only"); + + return Typecheck_Parameter(param, atom); +} diff --git a/src/include/datatypes/sys-sequence.h b/src/include/datatypes/sys-sequence.h index f2f933f862..b230ebbfe1 100644 --- a/src/include/datatypes/sys-sequence.h +++ b/src/include/datatypes/sys-sequence.h @@ -731,21 +731,6 @@ inline static bool IS_PREDICATE1_CELL(noquote(Cell(const*)) v) { return Get_Cell_Flag(v, REFINEMENT_LIKE); // !!! Review: test this first? } -inline static Symbol(const*) VAL_PREDICATE1_SYMBOL( - noquote(Cell(const*)) v -){ - assert(IS_PREDICATE1_CELL(v)); - return SYM(VAL_NODE1(v)); -} - -inline static bool IS_PREDICATE(Cell(const*) v) { - if (not IS_TUPLE(v)) - return false; - - DECLARE_LOCAL (temp); - return IS_BLANK(VAL_SEQUENCE_AT(temp, v, 0)); -} - inline static Symbol(const*) VAL_REFINEMENT_SYMBOL( noquote(Cell(const*)) v ){ diff --git a/src/include/datatypes/sys-typeset.h b/src/include/datatypes/sys-typeset.h index 6309267ae0..449b311a6d 100644 --- a/src/include/datatypes/sys-typeset.h +++ b/src/include/datatypes/sys-typeset.h @@ -142,7 +142,7 @@ inline static bool Matcher_Matches( #define PARAM_FLAG_REFINEMENT \ FLAG_LEFT_BIT(11) -#define PARAM_FLAG_PREDICATE \ +#define PARAM_FLAG_12 \ FLAG_LEFT_BIT(12) // Parameters can be marked such that if they are void, the action will not @@ -265,21 +265,29 @@ inline static REBPAR *Init_Param_Core( inline static REBVAL *Refinify(REBVAL *v); // forward declaration inline static bool IS_REFINEMENT(Cell(const*) v); // forward decl -inline static bool IS_PREDICATE(Cell(const*) v); // forward decl -// This is an interim workaround for the need to be able check constrained -// data types (e.g. PATH!-with-BLANK!-at-head being REFINEMENT!). See -// Startup_Fake_Type_Constraint() for an explanation. +inline static bool Is_Parameter_Unconstrained(noquote(Cell(const*)) param) { + return VAL_PARAMETER_ARRAY(param) == nullptr; // e.g. `[/refine]` +} + +inline static bool Is_Blackhole(Cell(const*) v); // forward decl + +// This does extra typechecking pertinent to function parameters, compared to +// the basic type checking. // -inline static bool Typecheck_Including_Constraints( +inline static bool Typecheck_Parameter( const REBPAR *param, Value(*) v // need mutability for ^META check ){ - if (VAL_PARAM_CLASS(param) == PARAM_CLASS_RETURN) { - if (Is_Pack(v)) - return true; // For now, assume multi-return typechecked it - if (Is_Raised(v)) - return true; // For now, all functions return definitional errors + if ( + GET_PARAM_FLAG(param, REFINEMENT) + or GET_PARAM_FLAG(param, SKIPPABLE) + ){ + if (Is_Nulled(v)) // nulls always legal...means refinement not used + return true; + + if (Is_Parameter_Unconstrained(param)) // no-arg refinement + return Is_Blackhole(v); // !!! Error_Bad_Argless_Refine(key) } // We do an adjustment of the argument to accommodate meta parameters, @@ -306,24 +314,6 @@ inline static bool Typecheck_Including_Constraints( if (Is_Nihil(v) and GET_PARAM_FLAG(param, VANISHABLE)) goto return_true; - // !!! Predicates check more complex properties than just the kind, and - // so will mess up on meta parameters. All of this needs review, but - // the main point is to realize that a frame built for a meta parameter - // has already "leveled up" and removed isotope status and added quotes, - // so the type checking must effectively unquote (if not actually do so, - // which may be the easiest approach) - - if ( - GET_PARAM_FLAG(param, REFINEMENT) - and IS_PATH(v) - and IS_REFINEMENT(v) - ){ - goto return_true; - } - - if (GET_PARAM_FLAG(param, PREDICATE) and IS_PREDICATE(v)) - goto return_true; - if (unquoted) Meta_Quotify(v); @@ -336,55 +326,3 @@ inline static bool Typecheck_Including_Constraints( return true; } - - -inline static bool Is_Parameter_Unconstrained(noquote(Cell(const*)) param) { - return VAL_PARAMETER_ARRAY(param) == nullptr; // e.g. `[/refine]` -} - -inline static bool Is_Blackhole(Cell(const*) v); // forward decl - -// During the process of specialization, a NULL refinement means that it has -// not been specified one way or the other (MAKE FRAME! creates a frame with -// all nulled cells). However, by the time a user function runs with that -// frame, those nulled cells are turned to BLANK! so they can be checked via -// a plain WORD! (not GET-WORD!). The exception is refinements--which -// treat null as the unused state (or state when null is explicitly passed). -// -// Note: This does not cover features like "skippability", "endability", -// dequoting and requoting, etc. Those are evaluator mechanics for filling -// the slot--this happens after that. -// -inline static void Typecheck_Refinement( - const REBKEY *key, - const REBPAR *param, - REBVAL *arg -){ - assert( - GET_PARAM_FLAG(param, REFINEMENT) - or GET_PARAM_FLAG(param, SKIPPABLE) - ); - - if (Is_Nulled(arg)) // not in use - return; - - if ( - Is_Parameter_Unconstrained(param) - and VAL_PARAM_CLASS(param) != PARAM_CLASS_OUTPUT - ){ - if (not Is_Blackhole(arg)) - fail (Error_Bad_Argless_Refine(key)); - - return; - } - - typecheck_again: - - if (not Typecheck_Including_Constraints(param, arg)) { - if (Is_Activation(arg)) { - Deactivate_If_Activation(arg); - goto typecheck_again; - } - fail (Error_Invalid_Type(VAL_TYPE(arg))); - } -} diff --git a/src/include/sys-eval.h b/src/include/sys-eval.h index 1b14186783..6cb987e426 100644 --- a/src/include/sys-eval.h +++ b/src/include/sys-eval.h @@ -224,7 +224,7 @@ inline static bool Did_Init_Inert_Optimize_Complete( // if (Get_Action_Flag(action, SKIPPABLE_FIRST)) { const REBPAR *first = First_Unspecialized_Param(nullptr, action); - if (not TYPE_CHECK(first, out)) + if (not Typecheck_Parameter(first, out)) goto optimized; // didn't actually want this parameter type }