LogtalkDotOrg / logtalk3

Logtalk - declarative object-oriented logic programming language
https://logtalk.org
Apache License 2.0
425 stars 31 forks source link

Strange "predicate as non-terminal" error #227

Closed cl-alexk closed 2 months ago

cl-alexk commented 2 months ago

Using latest git version a1d820b.

The code below results in Predicate called as a non-terminal: duration/3 even though it is defined as a non-terminal.

:- use_module(library(dcg/basics), []).
:- use_module(library(dcg/high_order), []).

:- object(iso_8601).
    :- use_module(dcg_basics, [number//1]).
    :- use_module(dcg_high_order, [sequence//2]).

    pdate(y, 31536000).
    pdate(m, 2592000).
    pdate(w, 604800).
    pdate(d, 86400).

    duration(S) -->
        "p", sequence(p, Ns),
        { numberlist::sum(Ns, S) }.

    p(N) -->
        number(N0), [C],
        { {atom_char(A, C)}, pdate(A, M), N is N0 * M}.

    :- public(duration_seconds/2).
    duration_seconds(Dur, Seconds) :-
        string_lower(Dur, Dur1),
        string_codes(Dur1, Codes),
        once(phrase(duration(Seconds), Codes)).
:- end_object.
pmoura commented 2 months ago

Using that Logtalk git version:

$ swilgt -q
?- logtalk_load(s, [optimize(on)]).
*     Reference to unknown object: numberlist
*       while compiling object iso_8601
*       in file /Users/pmoura/Downloads/s.lgt between lines 13-15
*     
true.

?- ^D
$ swilgt -q
?- logtalk_load(s, [optimize(off)]).
*     Reference to unknown object: numberlist
*       while compiling object iso_8601
*       in file /Users/pmoura/Downloads/s.lgt between lines 13-15
*     
true.

?- ^D
$ swilgt -q
?- logtalk_load(s, [debug(on)]).
*     Reference to unknown object: numberlist
*       while compiling object iso_8601
*       in file /Users/pmoura/Downloads/s.lgt between lines 13-15
*     
true.
pmoura commented 2 months ago
$ swilgt -q
?- {types(loader)}.
true.

?- {s}.
true.
cl-alexk commented 2 months ago

Hm, this must be an artefact of my loader configuration. Checking to see if this is minimally reproducible.

pmoura commented 2 months ago

Any implicit loading of other code, notably from a settings file? Try setting the portability flag to warning.

pmoura commented 2 months ago

Hm, this must be an artefact of my loader configuration. Checking to see if this is minimally reproducible.

That should have been the first thing to check before opening this issue.

cl-alexk commented 2 months ago

Ok, got it.

repro_hook.lgt:

:- object(h1, implements(expanding)).
    term_expansion(T,T) :- fail.
:- end_object.

:- object(h2, implements(expanding)).
    term_expansion(T,T) :- fail.
:- end_object.
% swilgt -q
?- {hook_flows(loader)}.
true.

?- {types(loader)}.
true.

?- {repro_hook}.
true.

?- set_logtalk_flag(hook, hook_pipeline([h1, h2])).
true.

?- {iso}.
*     Predicate called as a non-terminal: duration/3
*       while compiling object iso_8601

Also interesting:

% swilgt -q
?- {hook_flows(loader)}.
true.

?- {repro_hook}.
true.

?- set_logtalk_flag(hook, hook_pipeline([h1, h2])).
true.

?- {types(loader)}.
!     Type error: expected module_identifier but got library(unicode)
!       in directive use_module/2
!       in file /Users/user/logtalk/library/types/type.lgt at or above line 1310
pmoura commented 2 months ago

The hook_pipeline(Pipeline) object calls expand_term/2, which defaults to expand (-->)/2 terms outside the control of the linter, hence the warning. I.e. the grammar rules are being converted to clauses before the compiler and linter see them. Try:

:- object(h1, implements(expanding)).
    term_expansion((A --> B), (A --> B)).
    term_expansion(T,T) :- fail.
:- end_object.

:- object(h2, implements(expanding)).
    term_expansion((A --> B), (A --> B)).
    term_expansion(T,T) :- fail.
:- end_object.
pmoura commented 2 months ago

The second issue is fixed in fd3279089d0ff477ce70e166f97d363897fefbf4. Thanks for reporting.

cl-alexk commented 2 months ago

Is there a better solution (than instrumenting every hook in the pipeline to explicitly preserve nonterminals)?

Seems this is a problem only with hook_pipeline:

% swilgt -q
?- {types(loader)}.
true.

?- {repro_hook}.
true.

?- set_logtalk_flag(hook, h1).
true.

?- {repro_iso}.
true.

(unmodified h1, renamed file)

Besides, even with

:- object(h3, implements(expanding)).
    term_expansion((A --> B), (A --> B)).
    term_expansion(T,T) :- fail.
:- end_object.

there is still

% swilgt -q
?- {types(loader)}.
true.

?- {repro_hook}.
true.

?- {meta_compiler(loader)}. {hook_flows(loader)}.

true.

true.

?- set_logtalk_flag(hook, hook_pipeline([h3, meta_compiler])).
true.
?- {repro_iso}.
*     Predicate called as a non-terminal: duration/3
*       while compiling object iso_8601
pmoura commented 2 months ago

This is not a bug in the library. Your expansion rules are apparently too general instead of being as strict as possible as advisable. You're (implicitly) handling expansion of grammar rules instead of delegating it to the compiler/linter. If that's intentional, we should set the grammar_rules flag to silent and do your own linter checks.

cl-alexk commented 2 months ago

But why only in hook_pipeline and not in a standalone hook? Not a bug, but at least surprising.

I'm not intentionally expanding grammar rules, I'm rewriting some very specific clause heads and allowing it to fail when strict criteria are not met.

It looks like the main issue is confined to the case when a) term rewriting on a nonterminal can fail and b) it happens in hook_pipeline.

I was able to create a workaround in my most general expander, so thank you for pointing me in that direction:

    term_expansion((Head0 --> Body0), (Head --> Body)) :-
        expand(Head0, Body0, Head, Body).
    term_expansion((A --> B), (A --> B)).
    term_expansion((Head0 :- Body0), (Head :- Body)) :-
        expand(Head0, Body0, Head, Body).

The expand/4 is allowed to fail, but the expansion for non-terminals will always succeed, and non-terminals are always rewritten as nonterminals.

pmoura commented 2 months ago

Standalone hooks don't call expand_term/2, unlike hook_pipeline/1 and hook_set/1.

Pipeline steps should not fail, otherwise the whole pipeline would fail. Calls to the expand_term/2 method always succeed.

cl-alexk commented 2 months ago

Does that mean that every step must succeed, or any? The way I'm reading the documentation suggests the latter:

This parametric object is used when the expansions must be applied in a specific order. It also allows overriding the default compiler semantics where term-expansion rules are tried in sequence only until one of them succeeds.

But this discussion and what I'm seeing in suggests the former (i.e., a failing expansion in any step turns non-terminals into terminals).

Or is the doc saying that's the semantics that's being overridden? If so, it would be useful to see the overriding semantics stated.

Your expansion rules are apparently too general instead of being as strict as possible as advisable.

I have another step in the pipeline that is very specific (term_expansion(:- uses_ops(Module), Term)) and does not match anything else, including nonterminals. It appears to still need an explicit term_expansion((A --> B), (A --> B)). to prevent linter confusion.

Does this imply that any term expansion in a pipeline step is necessarily general?

cl-alexk commented 2 months ago

What is your recommendation here?

s.lgt:

:- object(o).
    foo --> [].
    bar :- phrase(foo, []).
:- end_object.

Just using meta_compiler in a pipeline by itself:

% swilgt -q
?- {hook_flows(loader)}.
true.

?- {meta_compiler(loader)}.
true.

?- set_logtalk_flag(hook, hook_pipeline([meta_compiler])).
true.

?- {s}.
*     Predicate called as a non-terminal: foo/2
*       while compiling object o
*       in file /Users/user/tmp/logtalk-repro/nonterminal-expansion/s.lgt at or above line 3
pmoura commented 2 months ago

There's a more serious issue here than the spurious linter warning: the library must not apply default expansions. For example, consider a pipeline of two hook objects where the first expands clauses and the second expands grammar rules. With the first hook object implicitly expanding grammar rules, the second hook object will never be used. Fixed in 88bcf39b9934f1e0ca223dd12cb3e14af494cca6.

cl-alexk commented 2 months ago

Confirmed resolved, thank you!