opencog / atomspace

The OpenCog (hyper-)graph database and graph rewriting system
https://wiki.opencog.org/w/AtomSpace
Other
815 stars 228 forks source link

Avoid using and testing for `Handle::UNDEFINED` #1511

Open ngeiswei opened 6 years ago

ngeiswei commented 6 years ago

I'm almost certain we don't want to allow things like

Inheritance
  Concept "A"
  Handle::UNDEFINED

yet Link::Link allows it. Shouldn't it check that every outgoing is defined and raise an exception if not? Or would that be too costly?

The reason I'm asking is because some code (specifically PutLink::reduce() but that's not the point) does create that kind of ill-formed link. Of course PutLink::reduce() needs to be fixed, but I wonder if a more general check shouldn't be added as well.

ngeiswei commented 6 years ago

I should mention that allowing this hurts while debugging because for instance Link::to_string() crashes, as well as Link::compute_hash. So basically if having such a check in Link::Link is too costly, which I doubt actually, I would instead make these above methods (to_string, compute_hash) more resilient to undefined atoms.

linas commented 6 years ago

Right; that's not allowed. what I've tried to do is to remove all usages of Handle::UNDEFINED in the code - that way, no check would be needed, because it would never be possible. Instead, code should just throw exceptions, instead of returning null pointers. So, in #1512, somehow all those null pointers should be throws, instead.

linas commented 6 years ago

FYI, there is a curious bit of category theory duality I learned of: exceptions are category theory opposites to error checks. One just "reverses the arrows". The arrows point either from past to future, or future to past. I don't know quite how to illustrate this. The if-checks-in-the-past are throws, the if-checks-in-the-future are error-code return values.

For the atomspace, I think it will be computationally better if we never-ever test for Handle::UNDEFINED and always throw instead. The idea is that the normal, error-free case is always fast.

linas commented 6 years ago

Also, insertion into the atomspace explicitly checks for undefined handles in links, and throws errors if they are found.

Is this still an issue? Can this issue be closed?

linas commented 6 years ago

Reading through the comments, I'm mthinking this issue should be redirected to a work item that says: "minimize the use of Handle::UNDEFINED in the code base, and try to remove most checks for it`. This should especially be the case in any performance-critical code sections.