* Bug 14435 fixed: better error reporting in extract overloads 86/21586/11
mottelet [Wed, 7 Oct 2020 10:14:55 +0000 (12:14 +0200)]
http://bugzilla.scilab.org/show_bug.cgi?id=14435

When you overload extraction on a MList or a TList, error reporting
should be handled by the overload, this is not the case:

t=tlist(["user","x"],0);
function varargout = %user_e(i,x)
  if or(i==["a" "b"])
    mprintf("Extract field %s\n",i);
   else
    error(msprintf("Error: field %s is undefined !\n\n",i));
   end
end

--> t.a
Extract field a

--> t.z
at line     5 of function %user_e

Undefined operation for the given operands.
check or define function %l_e for overloading.

I don't see the error message of the overload, it has been
superseeded. Moreover, it is incorrect (we are not overloading an
operator).

With the proposed patch we have:

--> t.z
at line     5 of function %user_e

Error: field z is undefined !

--> t("z")
at line     5 of function %user_e

Error: field z is undefined !

Change-Id: Idc8fa19579c38093019a97e8542f10ed4782ed8f

scilab/CHANGES.md
scilab/modules/ast/src/cpp/ast/runvisitor.cpp
scilab/modules/ast/src/cpp/types/mlist.cpp
scilab/modules/ast/src/cpp/types/overload.cpp
scilab/modules/ast/src/cpp/types/tlist.cpp
scilab/modules/ast/tests/nonreg_tests/bug_14435.tst [new file with mode: 0644]

index dfeb01f..4a1b0a9 100644 (file)
@@ -286,6 +286,7 @@ Bug Fixes
 * [#12889](https://bugzilla.scilab.org/12889): In the help browser, add a menu allowing to select the language of help pages, regardless of the language of the session.
 * [#13593](https://bugzilla.scilab.org/13593): `csvRead()` did not take the `range` into account when `header` is provided. `[]` could not be used as default `range`.
 * [#13762](https://bugzilla.scilab.org/13762): In the `fft` page, the formula for the inverse FFT missed the 1/n normalization factor.
+* [#14435](https://bugzilla.scilab.org/14435): Errors were not well handled in overloaded functions.
 * [#14488](https://bugzilla.scilab.org/14488): The `frameflag=9` and `strf=".9."` values of these `plot2d` options were no longer accepted. Their documentation was ambiguous.
 * [#14718](https://bugzilla.scilab.org/14718): `user` is removed for a while but was still documented.
 * [#14873](https://bugzilla.scilab.org/14873): `setfield` page: The output and the 6.0 history were documented only on the en_US version. The input was wrongly restricted to matrices, while any Scilab object is acceptable. The specific role of `setfield` for mlists was not really described nor illustrated. The example did not include any call to setfield.
index bad1233..b7dff73 100644 (file)
@@ -426,39 +426,34 @@ void RunVisitorT<T>::visitprivate(const FieldExp &e)
         in.push_back(pValue);
         types::Callable::ReturnValue Ret = types::Callable::Error;
         std::wstring stType = pValue->getShortTypeStr();
+        std::wstring wstrFuncName = L"%" + stType + L"_e";
 
         try
         {
-            Ret = Overload::call(L"%" + stType + L"_e", in, 1, out, true, true, e.getLocation());
+            Ret = Overload::call(wstrFuncName.c_str(), in, 1, out, false, false, e.getLocation());
+            if(Ret == types::Callable::OK_NoResult)
+            {
+                // overload not defined, try with the short name.
+                // to compatibility with scilab 5 code.
+                // tlist/mlist name are truncated to 8 first character
+                wstrFuncName = L"%" + stType.substr(0, 8) + L"_e";
+                Ret = Overload::call(wstrFuncName.c_str(), in, 1, out, false, true, e.getLocation());
+            }
         }
         catch (const InternalError& ie)
         {
-            try
+            // TList or Mlist
+            // last error is not empty when the error have been setted by the overload itself.
+            if (pValue->isList() && ConfigVariable::getLastErrorFunction().empty())
             {
-                //to compatibility with scilab 5 code.
-                //tlist/mlist name are truncated to 8 first character
-                if (stType.size() > 8)
-                {
-                    Ret = Overload::call(L"%" + stType.substr(0, 8) + L"_e", in, 1, out, true, true, e.getLocation());
-                }
-                else
-                {
-                    CoverageInstance::stopChrono((void*)&e);
-                    throw ie;
-                }
+                wstrFuncName = L"%l_e";
+                Ret = Overload::call(wstrFuncName.c_str(), in, 1, out, false, true, e.getLocation());
             }
-            catch (const InternalError& ie)
+            else
             {
-                // TList or Mlist
-                if (pValue->isList())
-                {
-                    Ret = Overload::call(L"%l_e", in, 1, out, true, true, e.getLocation());
-                }
-                else
-                {
-                    CoverageInstance::stopChrono((void*)&e);
-                    throw ie;
-                }
+                CoverageInstance::stopChrono((void*)&e);
+                // throw the exception in case where the overload have not been defined.
+                throw ie;
             }
         }
 
@@ -470,6 +465,21 @@ void RunVisitorT<T>::visitprivate(const FieldExp &e)
             throw InternalError(ConfigVariable::getLastErrorMessage(), ConfigVariable::getLastErrorNumber(), e.getLocation());
         }
 
+        // An extraction have to return something
+        if(out.empty())
+        {
+            setResult(NULL);
+            cleanInOut(in, out);
+            CoverageInstance::stopChrono((void*)&e);
+
+            wchar_t wcstrError[512];
+            char* strFuncName = wide_string_to_UTF8(wstrFuncName.c_str());
+            os_swprintf(wcstrError, 512, _W("%s: Extraction must have at least one output.\n").c_str(), strFuncName);
+            FREE(strFuncName);
+
+            throw InternalError(wcstrError, 999, e.getLocation());
+        }
+
         setResult(out);
         cleanIn(in, out);
     }
index ce1dd6d..33babfa 100644 (file)
@@ -115,13 +115,26 @@ bool MList::invoke(typed_list & in, optional_list & /*opt*/, int _iRetCount, typ
     this->IncreaseRef();
     in.push_back(this);
 
+    std::wstring wstrFuncName = L"%" + getShortTypeStr() + L"_e";
+
     try
     {
-        ret = Overload::call(L"%" + getShortTypeStr() + L"_e", in, _iRetCount, out);
+        ret = Overload::call(wstrFuncName, in, _iRetCount, out);
     }
-    catch (ast::InternalError & /*se*/)
+    catch (const ast::InternalError& ie)
     {
-        ret = Overload::call(L"%l_e", in, _iRetCount, out);
+        // last error is not empty when the error have been
+        // setted by the overload itself.
+        if (ConfigVariable::getLastErrorFunction().empty())
+        {
+            wstrFuncName = L"%l_e";
+            ret = Overload::call(wstrFuncName, in, _iRetCount, out);
+        }
+        else
+        {
+            // throw the exception in case where the overload have not been defined.
+            throw ie;
+        }
     }
 
     // Remove this from "in" for keep "in" unchanged.
@@ -133,6 +146,16 @@ bool MList::invoke(typed_list & in, optional_list & /*opt*/, int _iRetCount, typ
         throw ast::InternalError(ConfigVariable::getLastErrorMessage(), ConfigVariable::getLastErrorNumber(), e.getLocation());
     }
 
+    // An extraction have to return something
+    if(out.empty())
+    {
+        wchar_t wcstrError[512];
+        char* strFuncName = wide_string_to_UTF8(wstrFuncName.c_str());
+        os_swprintf(wcstrError, 512, _W("%s: Extraction must have at least one output.\n").c_str(), strFuncName);
+        FREE(strFuncName);
+        throw ast::InternalError(wcstrError, 999, e.getLocation());
+    }
+
     return true;
 }
 }
index 44a1da2..8caf65f 100644 (file)
@@ -97,27 +97,25 @@ types::Function::ReturnValue Overload::call(const std::wstring& _stOverloadingFu
                 return types::Function::ReturnValue::OK_NoResult;
             }
 
-            wchar_t pstError1[512];
+            char pstError1[512];
             char pstError2[512];
             char *pstFuncName = wide_string_to_UTF8(_stOverloadingFunctionName.c_str());
-            wchar_t* pwstError = NULL;
             if (_isOperator)
             {
                 os_sprintf(pstError2, _("check or define function %s for overloading.\n"), pstFuncName);
-                wchar_t *tmp = to_wide_string(pstError2);
-                os_swprintf(pstError1, 4096, L"%s%ls", _("Undefined operation for the given operands.\n"), tmp);
-                FREE(tmp);
+                os_sprintf(pstError1, "%s%s", _("Undefined operation for the given operands.\n"), pstError2);
             }
             else
             {
                 os_sprintf(pstError2, _("  check arguments or define function %s for overloading.\n"), pstFuncName);
-                wchar_t *tmp = to_wide_string(pstError2);
-                os_swprintf(pstError1, 4096, L"%s%ls", _("Function not defined for given argument type(s).\n"), tmp);
-                FREE(tmp);
+                os_sprintf(pstError1, "%s%s", _("Function not defined for given argument type(s),\n"), pstError2);
             }
-
             FREE(pstFuncName);
-            ast::InternalError ie(pstError1, 999, _location);
+
+            wchar_t* pwstError = to_wide_string(pstError1);
+            ast::InternalError ie(pwstError, 999, _location);
+            FREE(pwstError);
+
             ie.SetErrorType(ast::TYPE_EXCEPTION);
             throw ie;
         }
index aa6ffc7..f7c99ef 100644 (file)
@@ -184,29 +184,33 @@ bool TList::invoke(typed_list & in, optional_list & /*opt*/, int _iRetCount, typ
     in.push_back(this);
 
     std::wstring stType = getShortTypeStr();
+    std::wstring wstrFuncName = L"%" + getShortTypeStr() + L"_e";
+
     try
     {
-        ret = Overload::call(L"%" + stType + L"_e", in, _iRetCount, out);
+        ret = Overload::call(wstrFuncName, in, _iRetCount, out, false, false);
+        if(ret == types::Callable::OK_NoResult)
+        {
+            // overload not defined, try with the short name.
+            // to compatibility with scilab 5 code.
+            // tlist/mlist name are truncated to 8 first character
+            wstrFuncName = L"%" + stType.substr(0, 8) + L"_e";
+            ret = Overload::call(wstrFuncName, in, _iRetCount, out, false, true, e.getLocation());
+        }
     }
-    catch (const ast::InternalError &ie)
+    catch (const ast::InternalError& ie)
     {
-        try
+        // last error is not empty when the error have been
+        // setted by the overload itself.
+        if (ConfigVariable::getLastErrorFunction().empty())
         {
-            //to compatibility with scilab 5 code.
-            //tlist/mlist name are truncated to 8 first character
-            if (stType.size() > 8)
-            {
-                std::wcout << (L"%" + stType.substr(0, 8) + L"_e") << std::endl;
-                ret = Overload::call(L"%" + stType.substr(0, 8) + L"_e", in, _iRetCount, out);
-            }
-            else
-            {
-                throw ie;
-            }
+            wstrFuncName = L"%l_e";
+            ret = Overload::call(wstrFuncName, in, _iRetCount, out);
         }
-        catch (ast::InternalError & /*se*/)
+        else
         {
-            ret = Overload::call(L"%l_e", in, _iRetCount, out);
+            // throw the exception in case where the overload have not been defined.
+            throw ie;
         }
     }
 
@@ -219,6 +223,16 @@ bool TList::invoke(typed_list & in, optional_list & /*opt*/, int _iRetCount, typ
         throw ast::InternalError(ConfigVariable::getLastErrorMessage(), ConfigVariable::getLastErrorNumber(), e.getLocation());
     }
 
+    // An extraction have to return something
+    if(out.empty())
+    {
+        wchar_t wcstrError[512];
+        char* strFuncName = wide_string_to_UTF8(wstrFuncName.c_str());
+        os_swprintf(wcstrError, 512, _W("%s: Extraction must have at least one output.\n").c_str(), strFuncName);
+        FREE(strFuncName);
+        throw ast::InternalError(wcstrError, 999, e.getLocation());
+    }
+
     return true;
 }
 
diff --git a/scilab/modules/ast/tests/nonreg_tests/bug_14435.tst b/scilab/modules/ast/tests/nonreg_tests/bug_14435.tst
new file mode 100644 (file)
index 0000000..72d0370
--- /dev/null
@@ -0,0 +1,53 @@
+// ============================================================================
+// Scilab ( http://www.scilab.org/ ) - This file is part of Scilab
+// Copyright (C) 2020 - St├ęphane Mottelet
+//
+//  This file is distributed under the same license as the Scilab package.
+// ============================================================================
+
+// <-- CLI SHELL MODE -->
+// <-- NO CHECK REF -->
+
+// <-- Non-regression test for bug 14435 -->
+//
+// <-- Bugzilla URL -->
+// http://bugzilla.scilab.org/show_bug.cgi?id=14435
+//
+// <-- Short Description -->
+// Errors not well handled in overloaded functions
+
+t = tlist(["user","x"],0);
+m = mlist(["user","x"],0);
+
+message = [msprintf(_("Function not defined for given argument type(s),\n")); msprintf(_("  check arguments or define function %s for overloading.\n"), "%l_e")];
+assert_checkerror("t.z",message);
+assert_checkerror("m.z",message);
+assert_checkerror("t(""z"")",message);
+assert_checkerror("m(""z"")",message);
+
+function varargout = %user_e(i,x)
+    if or(i==["a" "b"])
+        varargout(1) = i + "1"
+        varargout(2) = i + "2"
+        return;
+    end
+
+    if i == "no_output" then
+        return;
+    end
+
+    error(msprintf("Error: field %s is undefined !",i));
+end
+
+assert_checkequal(t.a, "a1");
+assert_checkequal(t("a"), "a1");
+
+message = "Error: field z is undefined !";
+assert_checkerror("t.z",message);
+assert_checkerror("m.z",message);
+assert_checkerror("t(""z"")",message);
+assert_checkerror("m(""z"")",message);
+
+msg = msprintf(_("%s: Extraction must have at least one output.\n"), "%user_e");
+assert_checkerror("t.no_output", msg);
+assert_checkerror("t(""no_output"")", msg);
\ No newline at end of file