- try to preserve a bit more information about incorrect use of user variables to print more meaningful error messages. This is not complete yet and will need integration with the previous commit.

src/dobjtype.cpp

@@ -2791,6 +2791,7 @@ PClass::PClass()
 	Defaults = nullptr;
 	bRuntimeClass = false;
 	bExported = false;
+	bDecorateClass = false;
 	ConstructNative = nullptr;
 	mDescriptiveName = "Class";
 

src/dobjtype.h

@@ -775,6 +775,7 @@ public:
 	BYTE				*Defaults;
 	bool				 bRuntimeClass;	// class was defined at run-time, not compile-time
 	bool				 bExported;		// This type has been declared in a script
+	bool				 bDecorateClass;	// may be subject to some idiosyncracies due to DECORATE backwards compatibility
 	TArray<VMFunction*>	 Virtuals;	// virtual function table
 
 	void (*ConstructNative)(void *);

src/p_actionfunctions.cpp

@@ -78,6 +78,7 @@
 #include "p_spec.h"
 #include "templates.h"
 #include "vm.h"
+#include "v_text.h"
 #include "thingdef.h"
 #include "math/cmath.h"
 
@@ -132,6 +133,15 @@ bool ACustomInventory::CallStateChain (AActor *actor, FState *state)
 
 		if (state->ActionFunc != NULL)
 		{
+			if (state->ActionFunc->Unsafe)
+			{
+				// If an unsafe function (i.e. one that accesses user variables) is being detected, print a warning once and remove the bogus function. We may not call it because that would inevitably crash.
+				auto owner = FState::StaticFindStateOwner(state);
+				Printf(TEXTCOLOR_RED "Unsafe state call in state %s.%d to %s which accesses user variables. The action function has been removed from this state\n",
+					owner->TypeName.GetChars(), state - owner->OwnedStates, static_cast<VMScriptFunction *>(state->ActionFunc)->PrintableName.GetChars());
+				state->ActionFunc = nullptr;
+			}
+
 			VMFrameStack stack;
 			PPrototype *proto = state->ActionFunc->Proto;
 			VMReturn *wantret;

src/p_pspr.cpp

@@ -29,6 +29,7 @@
 #include "g_level.h"
 #include "d_player.h"
 #include "serializer.h"
+#include "v_text.h"
 
 
 // MACROS ------------------------------------------------------------------
@@ -337,6 +338,14 @@ void DPSprite::SetState(FState *newstate, bool pending)
 		{
 			FState *nextstate;
 			FStateParamInfo stp = { newstate, STATE_Psprite, ID };
+			if (newstate->ActionFunc != nullptr && newstate->ActionFunc->Unsafe)
+			{
+				// If an unsafe function (i.e. one that accesses user variables) is being detected, print a warning once and remove the bogus function. We may not call it because that would inevitably crash.
+				auto owner = FState::StaticFindStateOwner(newstate);
+				Printf(TEXTCOLOR_RED "Unsafe state call in state %s.%d to %s which accesses user variables. The action function has been removed from this state\n",
+					owner->TypeName.GetChars(), newstate - owner->OwnedStates, static_cast<VMScriptFunction *>(newstate->ActionFunc)->PrintableName.GetChars());
+				newstate->ActionFunc = nullptr;
+			}
 			if (newstate->CallAction(Owner->mo, Caller, &stp, &nextstate))
 			{
 				// It's possible this call resulted in this very layer being replaced.

src/sc_man.cpp

@@ -1046,7 +1046,10 @@ void FScriptPosition::Message (int severity, const char *message, ...) const
 {
 	FString composed;
 
-	if ((severity == MSG_DEBUG || severity == MSG_DEBUGLOG) && developer < DMSG_NOTIFY) return;
+	if (severity == MSG_DEBUGLOG && developer < DMSG_NOTIFY) return;
+	if (severity == MSG_DEBUGERROR && developer < DMSG_ERROR) return;
+	if (severity == MSG_DEBUGWARN && developer < DMSG_WARNING) return;
+	if (severity == MSG_DEBUGMSG && developer < DMSG_NOTIFY) return;
 	if (severity == MSG_OPTERROR)
 	{
 		severity = StrictErrors || strictdecorate ? MSG_ERROR : MSG_WARNING;
@@ -1073,9 +1076,11 @@ void FScriptPosition::Message (int severity, const char *message, ...) const
 		return;
 
 	case MSG_WARNING:
+	case MSG_DEBUGWARN:
+	case MSG_DEBUGERROR:	// This is intentionally not being printed as an 'error', the difference to MSG_DEBUGWARN is only the severity level at which it gets triggered.
 		WarnCounter++;
 		type = "warning";
-		color = TEXTCOLOR_YELLOW;
+		color = TEXTCOLOR_ORANGE;
 		break;
 
 	case MSG_ERROR:
@@ -1085,7 +1090,7 @@ void FScriptPosition::Message (int severity, const char *message, ...) const
 		break;
 
 	case MSG_MESSAGE:
-	case MSG_DEBUG:
+	case MSG_DEBUGMSG:
 		type = "message";
 		color = TEXTCOLOR_GREEN;
 		break;

src/sc_man.h

@@ -125,7 +125,9 @@ enum
 	MSG_FATAL,
 	MSG_ERROR,
 	MSG_OPTERROR,
-	MSG_DEBUG,
+	MSG_DEBUGERROR,
+	MSG_DEBUGWARN,
+	MSG_DEBUGMSG,
 	MSG_LOG,
 	MSG_DEBUGLOG,
 	MSG_MESSAGE

src/scripting/codegeneration/codegen.cpp

@@ -8073,7 +8073,7 @@ FxExpression *FxClassTypeCast::Resolve(FCompileContext &ctx)
 					delete this;
 					return nullptr;
 				}
-				ScriptPosition.Message(MSG_DEBUG, "resolving '%s' as class name", clsname.GetChars());
+				ScriptPosition.Message(MSG_DEBUGLOG, "resolving '%s' as class name", clsname.GetChars());
 			}
 		}
 		FxExpression *x = new FxConstant(cls, to, ScriptPosition);

src/scripting/codegeneration/codegen.h

@@ -79,6 +79,7 @@ struct FCompileContext
 	int StateIndex;			// index in actor's state table for anonymous functions, otherwise -1 (not used by DECORATE which pre-resolves state indices)
 	int StateCount;			// amount of states an anoymous function is being used on (must be 1 for state indices to be allowed.)
 	int Lump;
+	bool Unsafe = false;
 	TDeletingArray<FxLocalVariableDeclaration *> FunctionArgs;
 
 	FCompileContext(PFunction *func, PPrototype *ret, bool fromdecorate, int stateindex, int statecount, int lump);

src/scripting/decorate/thingdef_parse.cpp

@@ -909,6 +909,7 @@ PClassActor *CreateNewActor(const FScriptPosition &sc, FName typeName, FName par
 		}
 	}
 	ti = DecoDerivedClass(sc, parent, typeName);
+	ti->bDecorateClass = true;	// we only set this for 'modern' DECORATE. The original stuff  is so limited that it cannot do anything that may require flagging.
 
 	ti->Replacee = ti->Replacement = NULL;
 	ti->DoomEdNum = -1;

src/scripting/thingdef.cpp

@@ -206,6 +206,55 @@ void CreateDamageFunction(PClassActor *info, AActor *defaults, FxExpression *id,
 	}
 }
 
+//==========================================================================
+//
+// CheckForUnsafeStates
+//
+// Performs a quick analysis to find potentially bad states.
+// This is not perfect because it cannot track jumps by function.
+// For such cases a runtime check in the relevant places is also present.
+//
+//==========================================================================
+static int CheckForUnsafeStates(PClassActor *obj)
+{
+	static ENamedName weaponstates[] = { NAME_Ready, NAME_Deselect, NAME_Select, NAME_Fire, NAME_AltFire, NAME_Hold, NAME_AltHold, NAME_Flash, NAME_AltFlash, NAME_None };
+	static ENamedName pickupstates[] = { NAME_Pickup, NAME_Drop, NAME_Use, NAME_None };
+	TMap<FState *, bool> checked;
+	ENamedName *test;
+	int errors = 0;
+
+	if (obj->IsDescendantOf(RUNTIME_CLASS(AWeapon)))
+	{
+		if (obj->Size == RUNTIME_CLASS(AWeapon)->Size) return 0;	// This class cannot have user variables.
+		test = weaponstates;
+	}
+	else if (obj->IsDescendantOf(RUNTIME_CLASS(ACustomInventory)))
+	{
+		if (obj->Size == RUNTIME_CLASS(ACustomInventory)->Size) return 0;	// This class cannot have user variables.
+		test = pickupstates;
+	}
+	else return 0;	// something else derived from AStateProvider. We do not know what this may be.
+
+	for (; *test != NAME_None; test++)
+	{
+		FState *state = obj->FindState(*test);
+		while (state != nullptr && checked.CheckKey(state) == nullptr)	// have we checked this state already. If yes, we can stop checking the current chain.
+		{
+			checked[state] = true;
+			if (state->ActionFunc && state->ActionFunc->Unsafe)
+			{
+				// If an unsafe function (i.e. one that accesses user variables) is being detected, print a warning once and remove the bogus function. We may not call it because that would inevitably crash.
+				auto owner = FState::StaticFindStateOwner(state);
+				Printf(TEXTCOLOR_RED "Unsafe state call in state %s.%d to %s, reached by %s.%s which accesses user variables.\n",
+					owner->TypeName.GetChars(), state - owner->OwnedStates, static_cast<VMScriptFunction *>(state->ActionFunc)->PrintableName.GetChars(), obj->TypeName.GetChars(), FName(*test).GetChars());
+				errors++;
+			}
+			state = state->NextState;
+		}
+	}
+	return errors;
+}
+
 //==========================================================================
 //
 // LoadActors
@@ -253,6 +302,15 @@ void LoadActors ()
 			errorcount++;
 			continue;
 		}
+
+		if (ti->bDecorateClass && ti->IsDescendantOf(RUNTIME_CLASS(AStateProvider)))
+		{
+			// either a DECORATE based weapon or CustomInventory. 
+			// These are subject to relaxed rules for user variables in states.
+			// Although there is a runtime check for bogus states, let's do a quick analysis if any of the known entry points
+			// hits an unsafe state. If we can find something here it can be handled wuth a compile error rather than a runtime error.
+			errorcount += CheckForUnsafeStates(ti);
+		}
 	}
 	if (errorcount > 0)
 	{

src/scripting/vm/vm.h

@@ -656,6 +656,7 @@ class VMFunction : public DObject
 public:
 	bool Native;
 	bool Final = false;				// cannot be overridden
+	bool Unsafe = false;			// Contains references to class fields that are unsafe for psp and item state calls.
 	BYTE ImplicitArgs = 0;	// either 0 for static, 1 for method or 3 for action
 	int VirtualIndex = -1;
 	FName Name;