Remove nullfunc_t and make INVALID_FUNCTION the same as null.

INVALID_FUNCTION was proving to be a major pain; setting it to -1 might
be one of the dumbest things I've done. It had a few problems all
boiling down to "there is no support in the compiler for non-zero false
values". Eg, `f1 == f2` is false below:

        Function f1;
        Function f2 = INVALID_FUNCTION;

We could have gone through and fixed all the edge cases like this
throughout the compiler, but it seems counter-productive.

The other oddity is that because `null` is 0, `INVALID_FUNCTION` needed
a separate unit type, which we called `nullfunc_t`.

Rather than keep propping up all this weirdness, this patch normalizes
`INVALID_FUNCTION` by making it equivalent to `null`. Its value is 0,
and its type is `null_t`.

This will break compatibility with cross-plugin interactions, so plugins
sharing a scripted API will need to be recompiled.

It will also break C++ code relying on a -1 value. To help assist with
this breakage, three functions have been added to `IPluginContext`:

 - `GetNullFunctionValue()`
 - `IsNullFunctionId()`
 - `GetFunctionByIdOrNull()`.

README.md

@@ -185,3 +185,14 @@ Also as of this release, both the compiler and VM can run on non-x86 platforms
 ### SourcePawn 1.12
 
 SourcePawn 1.12 is currently in development.
+
+This release contains a number of bug fixes and internal improvements. It also greatly improves
+error message readability in the style of modern compilers.
+
+This release contains a number of language changes.
+ - Function bodies must now be braced. There is no compatibility mode for this change.
+ - `INVALID_FUNCTION` is now type `null_t`. The old internal type `nullfunc_t` has been removed.
+   `INVALID_FUNCTION` and `null` are now equivalent, which means code manually checking for `-1`
+    will not work. There are three functions added to `IPluginContext` to help native code with
+    this change: `GetNullFunctionValue()`, `IsNullFunctionId()`, and `GetFunctionByIdOrNull()`.
+ - Warnings around indentation have been removed.

compiler/assembler.cpp

@@ -924,7 +924,8 @@ Assembler::Assemble(SmxByteBuffer* buffer)
     code->header().main = 0;
     code->header().code = sizeof(sp_file_code_t);
     code->header().features = SmxConsts::kCodeFeatureDirectArrays |
-                              SmxConsts::kCodeFeatureHeapScopes;
+                              SmxConsts::kCodeFeatureHeapScopes |
+                              SmxConsts::kCodeFeatureNullFunctions;
     code->setBlob(cg_.code_ptr(), cg_.code_size());
 
     // Set up the data section. Note pre-SourceMod 1.7, the |memsize| was

compiler/expressions.cpp

@@ -290,20 +290,9 @@ matchobjecttags(Type* formal, Type* actual, int flags)
         return FALSE;
     }
 
-    if (actualtag == types->tag_nullfunc()) {
-        // All functions are nullable. We use a separate constant for backward
-        // compatibility; plugins and extensions check -1, not 0.
-        if (formal->isFunction())
-            return TRUE;
-
-        if (!(flags & MATCHTAG_SILENT))
-            report(154) << pc_tagname(formaltag);
-        return FALSE;
-    }
-
     if (actualtag == types->tag_null()) {
         // All objects are nullable.
-        if (formal->isObject())
+        if (formal->isFunction() || formal->isObject())
             return TRUE;
 
         // Some methodmaps are nullable. The nullable property is inherited
@@ -427,7 +416,7 @@ matchfunctags(Type* formal, Type* actual)
     if (formaltag == types->tag_function() && actual->isFunction())
         return TRUE;
 
-    if (actualtag == types->tag_nullfunc())
+    if (actualtag == types->tag_null())
         return TRUE;
 
     if (!actual->isFunction())

compiler/main.cpp

@@ -361,7 +361,7 @@ setconstants(void)
 {
     auto& cc = CompileContext::get();
     DefineConstant(cc, cc.atom("EOS"), 0, 0);
-    DefineConstant(cc, cc.atom("INVALID_FUNCTION"), -1, cc.types()->tag_nullfunc());
+    DefineConstant(cc, cc.atom("INVALID_FUNCTION"), 0, cc.types()->tag_null());
     DefineConstant(cc, cc.atom("cellmax"), INT_MAX, 0);
     DefineConstant(cc, cc.atom("cellmin"), INT_MIN, 0);
 

compiler/messages.h

@@ -178,7 +178,7 @@ static const char* errmsg[] = {
     /*151*/ "unmatched opening brace ('{') (line %d)\n",
     /*152*/ "no setter found for property %s\n",
     /*153*/ "Array-based enum structs have been removed. See https://wiki.alliedmods.net/SourcePawn_Transitional_Syntax#Enum_Structs\n",
-    /*154*/ "cannot assign INVALID_FUNCTION to a non-function type\n",
+    /*154*/ "unused154\n",
     /*155*/ "expected newline, but found '%s'\n",
     /*156*/ "invalid 'using' declaration\n",
     /*157*/ "'%s' is a reserved keyword\n",

compiler/types.cpp

@@ -131,7 +131,6 @@ TypeDictionary::init()
     type_void_ = defineVoid();
     type_object_ = defineObject("object");
     type_null_ = defineObject("null_t");
-    type_nullfunc_ = defineObject("nullfunc_t");
 }
 
 Type*

compiler/types.h

@@ -384,7 +384,6 @@ class TypeDictionary
             callback(type);
     }
 
-    Type* type_nullfunc() const { return type_nullfunc_; }
     Type* type_object() const { return type_object_; }
     Type* type_null() const { return type_null_; }
     Type* type_function() const { return type_function_; }
@@ -394,7 +393,6 @@ class TypeDictionary
     Type* type_bool() const { return type_bool_; }
     Type* type_string() const { return type_string_; }
 
-    int tag_nullfunc() const { return type_nullfunc_->tagid(); }
     int tag_object() const { return type_object_->tagid(); }
     int tag_null() const { return type_null_->tagid(); }
     int tag_function() const { return type_function_->tagid(); }
@@ -414,7 +412,6 @@ class TypeDictionary
     tr::unordered_map<Atom*, Type*> types_;
     tr::unordered_map<int, Type*> tags_;
     Type* type_int_ = nullptr;
-    Type* type_nullfunc_ = nullptr;
     Type* type_object_ = nullptr;
     Type* type_null_ = nullptr;
     Type* type_function_ = nullptr;

include/smx/smx-headers.h

@@ -78,6 +78,9 @@ struct SmxConsts {
 
     // This feature adds the HEAP_SAVE and HEAP_RESTORE opcodes.
     static const uint32_t kCodeFeatureHeapScopes = (1 << 2);
+
+    // This feature indicates that INVALID_FUNCTION is null (0) instead of -1.
+    static const uint32_t kCodeFeatureNullFunctions = (1 << 3);
 };
 
 // These structures are byte-packed.

include/sp_vm_api.h

@@ -1198,6 +1198,31 @@ class IPluginContext
      * EnterHeapScope.
      */
     virtual void LeaveHeapScope() = 0;
+
+    /**
+     * @brief Returns the value of a null function. If kCodeFeatureNullFunction
+     * is set, this returns 0. If unset, this returns -1, the legacy value.
+     */
+    virtual cell_t GetNullFunctionValue() = 0;
+
+    /**
+     * @brief Returns whether the value is a null function.
+     */
+    virtual bool IsNullFunctionId(funcid_t func) = 0;
+
+    /**
+     * @brief Wrapper around GetFunctionById/GetNullFunctionValue.
+     *
+     * If |func| is non-null and valid, |out| is set to the function and true
+     * is returned.
+     *
+     * If |func| is non-null and invalid, |out| is set to null and false is
+     * returned.
+     *
+     * If |func| is GetNullFunctionValue, |out| is set to null and true is
+     * returned.
+     */
+    virtual bool GetFunctionByIdOrNull(funcid_t func, IPluginFunction** out) = 0;
 };
 
 class AutoEnterHeapScope

tests/api/function-id.sp

@@ -0,0 +1,5 @@
+#include <shell>
+
+public main() {
+    printnum(execute(INVALID_FUNCTION, 1));
+}

vm/plugin-context.cpp

@@ -1101,14 +1101,36 @@ PluginContext::HeapAlloc2dArray(unsigned int length, unsigned int stride, cell_t
   return true;
 }
 
-void
-PluginContext::EnterHeapScope()
-{
+void PluginContext::EnterHeapScope() {
   enterHeapScope();
 }
 
-void
-PluginContext::LeaveHeapScope()
-{
+void PluginContext::LeaveHeapScope() {
   leaveHeapScope();
 }
+
+cell_t PluginContext::GetNullFunctionValue() {
+  auto image = runtime()->image();
+  if (image->DescribeCode().features & SmxConsts::kCodeFeatureNullFunctions) {
+    return 0;
+  }
+  return -1;
+}
+
+bool PluginContext::IsNullFunctionId(funcid_t func) {
+  return func == GetNullFunctionValue();
+}
+
+bool PluginContext::GetFunctionByIdOrNull(funcid_t func, IPluginFunction** out) {
+  if (IsNullFunctionId(func)) {
+    *out = nullptr;
+    return true;
+  }
+
+  *out = GetFunctionById(func);
+  if (!*out) {
+    ReportError("Invalid function id: 0x%08x", func);
+    return false;
+  }
+  return true;
+}

vm/plugin-context.h

@@ -25,7 +25,7 @@ static const cell_t STACK_MARGIN = 64; // 16 parameters of safety, I guess
 class Environment;
 class PluginContext;
 
-class PluginContext : public BasePluginContext
+class PluginContext final : public BasePluginContext
 {
  public:
   PluginContext(PluginRuntime* pRuntime);
@@ -61,6 +61,9 @@ class PluginContext : public BasePluginContext
                         const cell_t* init) override;
   void EnterHeapScope() override;
   void LeaveHeapScope() override;
+  cell_t GetNullFunctionValue() override;
+  bool IsNullFunctionId(funcid_t func) override;
+  bool GetFunctionByIdOrNull(funcid_t func, IPluginFunction** out) override;
   bool Invoke(funcid_t fnid, const cell_t* params, unsigned int num_params, cell_t* result);
 
   size_t HeapSize() const {

vm/shell/shell.cpp

@@ -185,22 +185,24 @@ static cell_t DoExecute(IPluginContext* cx, const cell_t* params)
 {
   int32_t ok = 0;
   for (size_t i = 0; i < size_t(params[2]); i++) {
-    if (IPluginFunction* fn = cx->GetFunctionById(params[1])) {
-      if (fn->Execute(nullptr) != SP_ERROR_NONE)
-        continue;
-      ok++;
-    }
+    IPluginFunction* fn;
+    if (!cx->GetFunctionByIdOrNull(params[1], &fn) || !fn)
+      return 0;
+    if (fn->Execute(nullptr) != SP_ERROR_NONE)
+      continue;
+    ok++;
   }
   return ok;
 }
 
 static cell_t DoInvoke(IPluginContext* cx, const cell_t* params)
 {
   for (size_t i = 0; i < size_t(params[2]); i++) {
-    if (IPluginFunction* fn = cx->GetFunctionById(params[1])) {
-      if (!fn->Invoke())
-        return 0;
-    }
+    IPluginFunction* fn;
+    if (!cx->GetFunctionByIdOrNull(params[1], &fn) || !fn)
+      return 0;
+    if (!fn->Invoke())
+      return 0;
   }
   return 1;
 }

vm/smx-v1-image.cpp

@@ -280,7 +280,8 @@ SmxV1Image::validateCode()
 
   uint32_t supported_features =
     SmxConsts::kCodeFeatureDirectArrays |
-    SmxConsts::kCodeFeatureHeapScopes;
+    SmxConsts::kCodeFeatureHeapScopes |
+    SmxConsts::kCodeFeatureNullFunctions;
   if (features & ~supported_features)
     return error("unsupported feature set; code is too new");