Revert back .equals() for Variable

[ssardina]

Sorry, I've only just noticed how you've changed Variable equality: I maintain that "A Variable is equal to another if their names are the same and they are not anonymous" and fear that you are restoring one of Fred's original misconceptions ;-/

A Java-side Variable is just a lexical token; it is most definitely not any variable within the Prolog engine. The object identity of a Variable doesn't mean much at all, and it matters not whether two occurrences of a variable within a goal (say, p(A, A)) are represented by the same (identical) Variable object or by two same-named instances of Variable - they are just lexical tokens.

Comparing two Variables Java-side is a not-particularly-useful operation whose only consistent definition is based on equality of their names; your version of equality won't break anything important, but it is wrong! If necessary I can elaborate until you are convinced!

Originally posted by @anionic in https://github.com/SWI-Prolog/packages-jpl/pull/52#issuecomment-623007912

[ssardina]

Hi @anionic ,

Thanks for commenting on that and happy to move it back.

I was completing the .hascode() for every class that had a .equals() and then I thought this was a glitch, seems not and this was thought before. As you can imagine I am not aware of all much discussion that has happened in the past haha

I can be convinced very easy don't worry. I am not going to fight for something you know much more than me. ;-)

You are right that comparing Variables is not useful operation and would almost never will be done. But of course we want still something that is as close to the "correct" meaning even if not used much. I agree one will never compare Variables.

So, can you explain me what is the intended behavior of this code (which I included in unit testing but I can change of course):

       Variable v1 = new Variable("X");
        Variable v2 = new Variable("X");

        Term s1 = new Query("? = 5", v1).oneSolution().get("X");
        Term s2 = new Query("? = 15", v2).oneSolution().get("X");

It appears to me that you want v1.equals(v2) to be True, despite the fact that they refer to different bindings (one 5 the other 150? Is that correct?

Seems you want to make them equal if they have the same textual name, full stop, is that correct?

[JanWielemaker]

Do you ma e a PR or tell me which commit to revert (or do it yourself)?

[ssardina]

I will do it, it is very easy.

But I want to wait for @anionic that he is happy with having the above behavior: two variables being equal but belonging to different queries and having different bindings.

[anionic]

Hi @ssardina

Please consider this variation of the code you quoted:

Variable v = new Variable("X");

Term s1 = new Query("? = 5", v).oneSolution().get("X");
Term s2 = new Query("? = 15", v).oneSolution().get("X");

Clearly, the variable used in the second query is identical to that used in the first: the variable doesn't "belong" to either query, and doesn't get bound to anything: it's just a source token. The returned bindings are keyed by the name of the variable in the query (just as in the top-level interpreter).

Variable instances with the same name are interchangeable, and thus equal.

This purely syntactic view of org.jpl7.Term makes possible e.g.

Query.oneSolution("append([a,b],[c],X)").get("X")

and if you abandon the principle that it is only the names of Variables which matter in terms, queries and bindings (which is deeply embedded in JPL's conversion between Term and term_t) then you lose this convenience.

Consider: new Atom("fred") doesn't create an atom in Prolog, it merely represents one in source text. Same goes for new Variable("X") ;-)

[ssardina]

Thanks @anionic .

I think I now get it fully. Yes! Terms as a whole are treated very syntactic and there shouldn't then be any further interpretation for Variables beyond their textual name. I have fixed that back and have improved the doc in many places to make this more explicit and elaborated (commit ce3448df13e49bb0165ad4eadee446779fb95a96)

One corner case: of course anonymous vars are not equal to any other var, but should an anonymous var be treated as .equals() to itself?

Variable v = new Variable("X");

boolean x = v.equals(v)  

In the original code they x will be FALSE. Under the strict textual interpretation I think that is correct as an anonymous var has "no name" (or a different name every time is used). A bit weird, but... Agree?

BTW, I am also elaborating the doc whenever I found there are gaps, always with the view of someone that is not expert on JPL and wants to use it.

[ssardina]

In noticed the check for .equals() was wrong for anonymous vars, because they are named _<number> but equality checks explicitly for not being _.:

public Variable() {
        this.name = "_" + Long.toString(n++); // e.g. _0, _1 etc.
    }

and the check for equals is:

public final boolean equals(Object obj) {
        return obj instanceof Variable && !this.name.equals("_") && this.name.equals(((Variable) obj).name);
    }

I will fix this later today. If any of the 2 are anonymous, then it should yield FALSE, but anonymous means _<number>

[anionic]

"dont-tell-me" variables aren't anonymous: see jpl7.org/ReleaseNotes300 which illustrates their use:

new Query("findall(_A,current_atom(_A),_As),length(_As,N)")

where it is vital that the two occurrences of _A are treated as one variable in query execution (ditto for _As), but we want their bindings to not be returned (unwanted, and could be huge, degrading performance).

_<integer> variables are legitimate (but bad style?) in user code, but normally only returned in queries, e.g.

?- copy_term([A,A,A], X).
X = [_4264, _4264, _4264].  % three occurrences (all equal) of a single new variable

?- length(X, 3).
X = [_4194, _4200, _4206].  % three new, unequal variables

where their equality is critical!

The Prolog engine never returns anonymous variables, either from the top-level interpreter or via JPL:

?- X = p(_), Y = q(X,X).
X = p(_3980),
Y = q(p(_3980), p(_3980)).

and they can always be avoided in user code, although sometimes admittedly convenient ;-)

[ssardina]

Correct. I know they are not good style, but we can create them via new Variable() in Java and we can obtained them indirectly from Java as your example suggests so we need to give them the intended equal() meaning.

I think I was a bit confused in all the ways one can get _ vars on the Java side, but the fact is that we can get them all. The anonymous var can only be obtained by doing new Variable("_"). Using new Variable() will give dont-tell vars numbered sequentially.

I have added many unit testing covering all cases, many of those tests taken from your examples above. The Variable.equals() is now:

@Override
public boolean equals(Object o) {
    if (o == null || getClass() != o.getClass() || this.name.equals("_") || ((Variable) o).name.equals("_")) return false;
    if (this == o) return true;
    Variable variable = (Variable) o;
    return name.equals(variable.name);
    }

So, anonymous var _ is always different to anything, including itself.

All other vars fall back to their textual name being equal.

[JanWielemaker]

I've been away from mail a couple of days. If I scan this briefly I think this is work in progress and I should wait testing/merging, no?

[ssardina]

No need to do much, I just reverted back the .equals() for variables as per @anionic comment. And I did push it to master as it was safe and a revert.

What is work in progress is #54 , because I found out that current_prolog_flag now returns a dictionary in one of the instances and dicts are not handled at JPL. They are compounds. In Java they are Map which are very used data structures in Java. So I am adding that to JPL, I am 70%. That will need a PR, but not quite yet.