Instantiator::walk_tree doesn't call the correct atom type constructor after subtitution

[ngeiswei]

Towards the end of

Handle Instantiator::walk_tree(const Handle& expr)

a new link is constructed with its outgoings substituted by the arguments passed to the ExecutionOutputLink. See

    if (changed)
        return Handle(createLink(t, oset_results, expr->getTruthValue()));

However, createLink merely call the Link constructor, by-passing the possibly useful C++ inheritance hierarchy of atoms (ScopeLink, FunctionLink, etc).

The consequences is that when walk_tree recreates the ExecutionOutputLink right before execution, see

        ExecutionOutputLinkPtr eolp(createExecutionOutputLink(sn, args));
        return eolp->execute(_as);

the call

void FreeLink::init(void)
{
    _vars.find_variables(_outgoing);
}

is unable to properly ignore the bound variables after substitution. For instance in the following conditional, the conditional is always true

            ScopeLinkPtr sco(ScopeLinkCast(lll));
            if (NULL == sco)
                sco = createScopeLink(lll->getOutgoingSet());

I'm not exactly sure what is the right fix. My guess is that createLink in Instantiator::walk_tree should be replaced by a AtomTable::factory, AtomTable::clone_factory or something like that. But I'm not enough confident to be sure.

[ngeiswei]

Hmmm, it seems I could just wrap AtomTable::factory around createLink but I can't access the atomtable (get_atomtable is private). There is a static AtomTable::do_clone_factory but it's not declared in the header... Puzzled...

I think meanwhile I'll just add the atom in the atomspace via add_link, but I won't commit anything, just do that so I can move forward...

[ngeiswei]

Wrapping add_atom around the createLink works, but I won't commit anything...

[ngeiswei]

It has a side effect though, the following 2 unit tests are failing

The following tests FAILED:
     52 - ArcanaUTest (Failed)
     58 - EvaluationUTest (Failed)

Which isn't too surprising, since it adds something new in the AtomSpace it creates rooms for altering the behavior.

I suppose we want to have something like add some method in the atomspace (maybe a static one), like create_atom...

[linas]

Yep. The factories are mildly headache-inducing, in part due to the circular shared-library linkages that result. Probably, the factories should be moved out of the atomspace library, to .. somewhere else. Maybe something local, I experimented with a generic factory for everything that is a FunctionLink (see FunctionLink.h)

placing the new atom in the scratch atomspace should have solved the problem, without braking any unit tests. The whole idea of the scratch space is to hold temporaries...

I don't ave any particularly good answers; if you happen to see some clean or elegant or generic way to do this, that's OK with me...

[ngeiswei]

@linas is there a special scratch atomspace, or should I construct one on the go? (I can see that the scheme evaluator uses one, but this issue isn't tied to scheme). Thanks.

[ngeiswei]

@linas Actually ArcanaUTest fails because it uses ImplicationLink which are now (with my local changes) ill-formated, like

(ImplicationLink
    (ListLink
        (EvaluationLink
            (PredicateNode "this way")
            (ListLink
                (ConceptNode "this one")
                (ConceptNode "thing two")
            )
        )
    )
    (ListLink
        (EvaluationLink
            (PredicateNode "this way")
            (ListLink
                (ConceptNode "this one")
                (ConceptNode "thing two")
            )
        )
    )
    (ListLink
        (EvaluationLink
            (PredicateNode "this way")
            (ListLink
                (ConceptNode "this one")
                (ConceptNode "thing two")
            )
        )
    )
)

I'm tempted to just remove them but then maybe your unit test is gonna miss useful cases. Anyway, I guess I'll proceed but I won't merge until you take a look at it.