Exceptions improvement

[abdullah]

We have been discussing about this in this thread:
https://forum.freecadweb.org/viewtopic. ... 30#p171864

After considering the input, I have decided to structure this improvement of exceptions into 3 stages in at least 3 PR:
1. Improve exceptions so that function/file/line number information is added in the report view.
2. Improve exceptions so that a translatable message is used in the UI.
3. Improve exceptions so that no information is lost, the message is not modified because the exception traveling from c++ to python.

This should allow not to diverge too much and have much more tight code reviews of such a core code. Which should benefit everybody.

[abdullah]

First PR - stage 1

https://github.com/FreeCAD/FreeCAD/pull/721

1. Enable automatic harvesting of information (function, file, line) when throwing the exception via macro:

Examples:

THROWM(Exception, "BSpline GeoId is out of bounds.")
THROWM(ValueError, "BSpline GeoId is out of bounds.")

THROW(AbortException)

Output:

a) Python Console (what()):
App.ActiveDocument.Sketch004.modifyBSplineKnotMultiplicity(16,3,0)
Traceback (most recent call last):
File "<input>", line 1, in <module>
Base.FreeCADError: FreeCAD exception thrown (BSpline GeoId is out of bounds.)

b) Report View (report()):
Exception (Thu Apr 27 19:15:24 2017): BSpline GeoId is out of bounds. in bool Sketcher::SketchObject::modifyBSplineKnotMultiplicity(int, int, int) in src/Mod/Sketcher/App/SketchObject.cpp:4102

2. Extend the basic framework so as to allow more control over the mangling of the message introduced by the user, setting the basis to allow, where needed, to preserve the original message while allowing full legacy behaviour.

From the related discussion thread:

This pull request does not deal with translations.

After reconsidering previous topics and looking at the error messages of some rare translations (like access violation), I have reached the conclusion that potentially any exception type can have a meaningful message for a UI user (in that case it was save your work and close freecad, or something similar, which is very relevant for a user who might lose his work). For sure, I still maintain that not every translation is to be translated (and for rare exception types it is rather the exception, not the rule to have a non-very-technical message useful for the user). However, in view of this, I am going to try in stage 2 to make this happen without having to create any specialized exceptions at all. Just reusing the infraestructure of stage1. Let's see if I manage.

Let me know your impressions.

[triplus]

I have reached the conclusion that potentially any exception type can have a meaningful message for a UI user (in that case it was save your work and close freecad, or something similar, which is very relevant for a user who might lose his work).

Report view should have meaningful (including regular user related) messages that much is true. But i would imagine some users don't have it opened and rather leave the internals of FreeCAD to autosave mechanism (to deal with it).

[abdullah]

... summoning

I have been looking to your containers exception mechanism (to basically see if I can get inspired for the main exception one). I see how you cleverly set the right python objection from its c++ counterpart.

How are you using these python exceptions? I mean, are you differentiating between types somewhere at returning from python execution?

I mean, I guess you must use the interpreter to execute any python command from c++, so then you execute your python command that eventually calls the c++ code, that generates a c++ exception, that cleverly gets converted into a python exception of the matching type, which gets returned to the interpreter...

...but you did not modify the interpreter and it only checks for PyExc_SystemExit internal python exception, the rest are processed via pyException. So you must be using the pyException mechanism...

...you are indeed catching Py::Exception in a couple of places, but you just Report the exception.

I am missing something?

[DeepSOIC]

How are you using these python exceptions?

I don't, yet. It's an API, and it's handy when you can distinguish exceptions from python code. I haven't established a mechanism to convert py exceptions back into the same c++ ones.

[abdullah]

How are you using these python exceptions?

I don't, yet. It's an API, and it's handy when you can distinguish exceptions from python code. I haven't established a mechanism to convert py exceptions back into the same c++ ones.

Ok. Sure is handy! I understand now. I thought I was missing something

[abdullah]

Second PR - Stage 3 (yes I skipped Stage 2)

This PR:
https://github.com/FreeCAD/FreeCAD/pull/739

Proof of concept. For review.

It is a possible solution to bypass information via python using setObject instead of setString, while still being compatible with any objection raised using setString. I uses a Python Dictionary to convey the c++ extra information thru python, so as to enable reconstruction of the original c++ exception after Python.

I skipped Stage 2, because it makes more sense to me today to solve this problem first.

[CADennis]

I am not a fan of communicating to the end user through exceptions. That makes coding for fellow developers feel like flying through a meteroid field. The argument, it would reduce necesary branch coverage testing, is only valid to those who do not test the exception branches. A cheap trick to justify test gaps. Thus, the more often you use that as a "user interface", the bigger the technical debt.

Please find a solution that does not introduce additional macros (c.f. motivation for modules by Gabried Dos Reis) and that allows me as a developer, for every line where I want to throw something, to introduce a unique exception class with the naming scheme

Code: Select all

<hosting class>_ERR_<GUID without hyphens>

e.g.

Code: Select all

AttachExtension_ERR_84d3f27d1c234981a53g2468h31eacf5
AttachExtension_ERR_8h22e39ecdab423g81a7847622339cb7a
AttachExtension_ERR_1a92d667f5g24451b4221742383082b5

Having functions return structs / enums / documented integers / optionals for other coders makes method contracts explicit and keeps them local. Throwing precise technical error IDs instead of source lines is in time of GIt great for issue handling and recognition of duplicates or regressions. The user can receive all relevant output through log/console instructions, not through exception stack traces. Let's conquer emergency situations via regular autosave and RAII, not by asking the user to extract content from the system when it is already in a corrupted state.

Thank you and kind regards
Dennis

[source: https://forum.freecadweb.org/viewtopic. ... 99#p171199, ...] Note that I had to comment out the catch(Base:Exception).

Why?

I had to do that because:
1. The one ultimately controlling the setting of the flag (and that will clear it if not receiving what it expects) is: Document::_recomputeFeature(DocumentObject* Feat) #5
2. This function will reset any flag set unless: a) the return code is not DocumentObject::StdReturn or b) a Base:Exception was thrown during the execution of the Feature->recompute() #4
3. Feature->recompute() #4 will return whatever #3 returned
4. #3 will return whatever #2 returned
5. #2 will return whatever #1 returned
6. #1 always, unconditionally returns StdReturn
7. at #0, the commented Base::Exception would catch my Base::ValueError exception that is derived from Base::Exception, thereby removing any hope of propagating the error information to #5

Now somebody took extraordinary care to catch all possible exceptions in #0 and return an "App::DocumentObjectExecReturn" which is nevertheless ignored in #1.

And that is the ultimate reason why I am writing this. Is there any reason for this? Can I just propagate all Base::Exceptions as with the commented code?

I also can catch first a Base::ValueError to re-throw the same and avoid the catching by Base::Exception that may be needed for other exceptions, but in such a case I am not returning the "App::DocumentObjectExecReturn". [...]

exactly

[wmayer]

Continuing the thread from the PR:

Since the suggestion in https://github.com/FreeCAD/FreeCAD/pull ... 86295415df isn't a convenient way and also doesn't support exception types of external modules I propose a factory class based on templates.

In FreeCADBase we have the class AbstractProducer. Now the template sub-class can look like this:

Code: Select all

struct ExceptionInfo {
   std::string typename;
   std::string function;
   std::string message;
   ...
};

class AbstractExceptionProducer : public AbstractProducer
{
public:
    AbstractExceptionProducer () {}
    ~AbstractExceptionProducer() {}
    // just implement it
    void* Produce () const {
        return nullptr;
    }
    virtual void raiseException(const ExceptionInfo& info) const = 0;
};

template <class CLASS>
class ExceptionProducer : public AbstractExceptionProducer
{
public:
    ExceptionProducer ()
    {
        ExceptionFactory::instance()->AddProducer(typeid(CLASS).name(), this);
    }

    virtual ~ExceptionProducer (){}

    void raiseException(const ExceptionInfo& info) const
    {
        CLASS c;
        // set data from info
        ...
        throw c;
    }
};

Code: Select all

// register exception types
new ExceptionProducer<Base::Exception>;
new ExceptionProducer<Base::XMLBaseException>;
new ExceptionProducer<Base::XMLParseException>;
...

Code: Select all

// catch and save the original exception type
...
catch (const Base::Exception& e) {
    std::string type = typeid(e).name();
    ...
    // pass info to Python
    PyErr_SetObject(...);
    return 0;
}

Code: Select all

// Restore the C++ exception
Base::ExceptionInfo info = ... // get from Python structure
Base::ExceptionFactory::instance()->raiseException(info);

[abdullah]

I propose a factory class based on templates.

and not that you usually don't rock, but here you really rock

I will give it a more attentive look and try to produce a good full solution