Fix dcgs using call(M:Pred) when M was left unassigned

[adri326]

Fixes #2725.

phrase(GRBody, _, _) first calls strip_module(GRBody, M, _) to split GRBody into the module and the predicate, but then calls call(M:GRBody3) to prepend the module again.

However, when there is no module, then strip_module doesn't assign a value to M.

I first tried to fix this by making it so that strip_module(predicate, M, Pred) assigns [] to M, but that seems to have broken several pieces of code that instead check whether or not M was assigned.

Right now I have defaulted to just handle the result of strip_module in phrase instead, but I feel like it would be much cleaner to allow strip_module to assign a value to M and to work both ways. call could then seamlessly work with it.

As per the suggestions of Markus, strip_module/3 now unifies the second argument with the default module, user.
I did this by introducing a new predicate, default_module/1, which does this unification, using the same logic as machine.quantification_to_module_name(ModuleQuantification::Unspecified).

With this PR, strip_module/3 now calls load_context(M) in the unspecified case.
This lets us remove fixes around the previous behavior (namely some uses of strip_subst_module/4).
A couple of test cases validate that strip_module/3 and DCGs now work as expected.

[triska]

Thank you a lot for working on this!

I think your first approach had the right intuition: The fix seems better placed in strip_module/3 so that other libraries also benefit from this. For goals that are not module-qualified, strip_module/3 should either yield the context module (if present), or the default module, user.

Example, for the case of no context module, i.e., used directly on the toplevel:

?- strip_module(G0, M, G).
   G0 = G, unsound, unexpected.
   M = user, G0 = G. % expected

[triska]

Thank you a lot! Could you please consider fusing the first two commits, since the first commit in this sequence is no longer needed?

[adri326]

I'm indeed happier with that solution:

?- strip_module(G, M, P).
   G = P, M = user.
?- strip_module(hello, user, P).
   P = hello.
?- default_module(Default).
   Default = user.

I've introduced '$default_module'/1 and default_module/1, which outputs the module name used when ModuleQuantification::Unspecified is encountered during module resolution, and used it in strip_module.

[adri326]

(squashed)

[triska]

For tests, please consider a test case that uses a module other than user. For instance, with m.pl consisting of:

:- module(m, [f/1]).

f(M) :-
        strip_module(goal, M, goal).

We expect:

$ scryer-prolog m.pl
?- f(M).
M = m.

[triska]

There are now still changes to dcgs.pl, are they still needed even with the improved semantics of strip_module/3?

[adri326]

For tests, please consider a test case that uses a module other than user. For instance, with m.pl consisting of:

:- module(m, [f/1]).

f(M) :-
        strip_module(goal, M, goal).

We expect:

$ scryer-prolog m.pl
?- f(M).
M = m.

Shouldn't it return user in the REPL, and only m when called within the module? Otherwise predicates like phrase would incorrectly add dcgs: as a prefix

[triska]

My understanding of this: A goal Goal which appears in module M is meant to refer to a predicate in its context module. This is what we expect from a goal that appears in any module: That it is called within that module. For this reason, I think M should be m in the case above, because m is the context module of goal.

Whether the indicated predicate stems from a library seems to make no difference regarding this specific point (i.e., regarding context modules). For instance, if the goal phrase(nt, Ls) appears in any module M, it is meant to indicate M:phrase(nt, Ls), i.e., a call of phrase/2 in the context of that module, which can also resolve to a predicate defined in a library which that module imports. For this reason, I would expect M to be (again) m also for a goal like phrase in the case above, since m is the context module of the goal.

Are there any other opinions on this, and corrections on this? (@UWN please?)

As an aside, phrase/2 is also declared as a meta-predicate, and that affects the inner argument nt, meaning that overall the goal is then: M:phrase(M:nt, Ls).

[adri326]

Hmm, that would mean that we would now have to differentiate between atoms that appear within a module and atoms that appear outside of it? Right now if we have a module that exports an atom, then it's equivalent to other atoms:

:- module(bello, [bello_to_you/1]).

bello_to_you(X) :-
    X = bello.

Then the following query should either fail (if both atoms are considered distinct), or M1 = M2 = user.

?- bello:bello_to_you(B), B = bello, strip_module(B, M1, _), strip_module(bello, M2, _).

Otherwise it doesn't make much sense to me that two values which unify would behave differently

[triska]

we would now have to differentiate between atoms that appear within a module and atoms that appear outside of it?

No, definitely not. An atom is always the same atom.

Right now if we have a module that exports an atom, then it's equivalent to other atoms:

Yes, absolutely, this is how it should be.

a module that exports an atom

Via the module/2 directive, a module exports predicates by stating their predicate indicators. A predicate name is only coincidentally represented by an atom, let us not confuse predicates with atoms or with predicate arguments. We are here focusing exclusively on module-qualifying goals, and (with the exception of the arguments of the predicate strip_module/3 itself) not discussing any arguments of predicates.

In your example, the predicate used by bello_to_you/1 is the built-in predicate (=)/2, and the goal that appears in its definition is: =(X, hello). We understand any module-unqualified goal that appears in a predicate definition to refer to predicates in the module where the predicate is being defined. In this concrete case, (=)/2 is a built-in predicate and not defined within the module bello, but we still understand that any goal that appears in the definition of a predicate is meant to refer to predicates within that module.

The arguments of goals are not affected in any way by strip_module/3, its point is only to extract the module in which a given goal should be called. For a module-qualified goal, that module is the innermost specified module. For a goal that is not module-qualified, that module is the module in which the goal appears. If the goal does not appear in any module (i.e., in a file without a module/2 declaration), that module is user.

In the example you gave, strip_module/3 is used on the toplevel. In the example I gave, it occurs within the context of module m.

[triska]

Already now, Scryer Prolog must be in a sense aware of the compilation unit (module) it currently compiles or runs, in order to resolve calls correctly. I believe it is that context module that strip_module/3 should yield as its second argument, if invoked for a module-unqualified goal during compilation or execution of predicates in that module, because that module is meant when an unqualified goal is encountered in that module and that module is therefore most useful to acquire via strip_module/3. But please do think it through carefully also, there may be a mistake in this view!

[triska]

Regarding the last post, it just occurred to me that it may be possible to resolve this at least partly by declaring strip_module/3 as a meta-predicate in such a way that the context module is automatically added to the first argument during compilation.

For instance, if we simulate this effect with custom_strip_module/3:

:- module(m, [f/1]).

:- meta_predicate(custom_strip_module(0, ?, ?)).

custom_strip_module(Goal0, M, Goal) :-
        strip_module(Goal0, M, Goal).

f(M) :-
        custom_strip_module(goal, M, goal).

Then it already works as expected also with your branch @adri326!

?- f(M).
   M = m.

What else is needed? Let's consider how strip_module/3 is used by library(dcgs): I think it must acquire the intended context module also during compilation of a module, and specifically during goal expansion, even when strip_module/3 does not itself occur in that module.

[adri326]

Oh, I see what you mean.

I think that strip_module fulfills a broader usecase and it shouldn't attempt to resolve atoms as predicates nor become a meta-predicate. I can think of three main ways in which it would be used:

• Like you intend to, to figure out the module in which a predicate is defined, either to make interfacing with rust code easier, or to do some minor modifications to the predicate, like dcgs:phrase. In this case, the predicate pred using strip_module is a meta-predicate, and the module resolution should (and does) happen when pred is called, at the boundary from non-meta- to meta-predicate.
• In "unwise" scenarios, where strip_module is used to later make major modifications to the argument, which is thus encoded as an atom. Users could be educated in the documentation of strip_module that this kind of usage could run into issues should the atom later cross a meta-predicate boundary and be resolved from within the module.
• In "exotic" scenarios, where the predicate is expected to be resolved from within a specific module. In this case, users could make sure that the meta-predicate boundary happens in the right module before calling strip_module.

In all three of these scenarios, the current behavior of strip_module can be kept as-is. The logic for resolving modules of predicates would happen exclusively when an atom is passed to a meta-predicate (ie. it crosses this kind of boundary), and gets cast to a predicate.

Doing this would ensure that no funniness arises when a module calls a meta-predicate like dcgs:phrase with a private predicate:

:- module(game_loader, [load_game/1]).
:- use_module(library(dcgs)).

game_dcg(Game) --> Game.

load_game(Game) :- phrase(game_dcg(Game), "This is just an example").

If strip_module attempts to do the resolution itself, then it wouldn't be able to find the module in which game_dcg is defined (or worse, it would guess incorrectly should the user define and import game_dcg somewhere else).

But if the module resolution happens when phrase is called (since it is a meta-predicate), then phrase will instead receive game_loader:game_dcg as argument and pass it to strip_module.

Right now this is largely the way in which this works, although I've found some edge cases in testing, which I would like to address in another issue.

I think that we should then document how strip_module works together with meta-predicates and how module resolution works when calling a meta-predicate.

[adri326]

Hmm, looks like metapredicates are meant to default to adding the calling module as a prefix, even though the predicate passed as argument might originate from another module.

This is kind of necessary for assert to work, but it shouldn't break anything, since if the module is imported, its predicates should be accessible from the parent module, even with the parent: prefix.

It does mean that there is a bug in module resolution. Given the following module:

:- module(module_resolution, [get_module/2, some_predicate/2]).

:- meta_predicate(get_module(:, ?)).
get_module(Pred, M) :- strip_module(Pred, M, _).

some_predicate(X, Y) :- X is Y + 1.

The query from within the user module ?- get_module(some_predicate, M) should return M = user (which it does, and user:some_predicate(X, Y) is rightfully callable), and the query from within the user module ?- module_resolution:get_module(some_predicate, M) should return M = user (which it doesn't, right now it returns M = module_resolution).

[hurufu]

You consider adding meta-predicate declaration with : specifier. It will inform that first argument isn't going to be called in any way.

[triska]

Let's consider this concrete use of strip_module/3 in library(dcgs):

scryer-prolog/src/lib/dcgs.pl

Line 262 in d57f871

loader:strip_module(GRBody, M, GRBody0),

This occurs during goal expansion, when a specific module (such as m) is compiled. Note that even though this expansion-rule is defined within library(dcgs), it is defined as a global expansion rule (via its user: prefix), and thus affects all code that is compiled thereafter.

What we expect from this use of strip_module/3 is that it reliably tells us the module M in which the indicated GRBody (the first argument of the expansion rule) ought to be invoked. If GRBody is not module-qualified, then it is meant to refer to goals in the context module, i.e., in the module that is being compiled.

Note that atoms are not ever associated with any module in Scryer Prolog: Atoms and functors, and in fact all terms, remain the same over module boundaries. I mention this because there are Prolog systems that implement so-called functor based module systems where two terms may have the exact same shape and appearance yet be distinct because they stem from different modules. This is not the case in Scryer Prolog. All that strip_module/3 is ever given is a goal (represented as a Prolog term), and that goal may be module-qualified or not, and we expect strip_module/3 to tell us the module that the goal is meant to refer to. An unqualified goal that is compiled within the context of a module M is meant to refer to a goal in M.

[triska]

and the query from within the user module ?- module_resolution:get_module(some_predicate, M) ... right now it returns M = module_resolution

I believe the current behaviour is correct. Any goal of the form module_resolution:Goal refers to a goal within the module module_resolution, since the prefix module_resolution: is the innermost module qualifier. As a consequence, a goal of the form user:module_resolution:Goal also refers to a goal in the module module_resolution, since the innermost qualification overrides the outer one(s).

It is this mechanism that makes it possible to explicitly indicate the intended module of a goal, even in situations where the goal is later qualified (automatically) with further prefices. For instance, within module x, when I write: maplist(m:goal, ...) then I make it clear that I intend to call m:goal, even if the meta_predicate/1 declaration of maplist/2 will turn this into x:m:goal because it adds the qualifier of the context module, i.e., x:. The innermost qualifier m overrides x.

---

To make discussing such issues easier: We can describe expected and unexpected behaviour of Scryer Prolog, via toplevel interactions. For instance, to make the case above more compact and also testable, I could say:

?- module_resolution:get_module(some_predicate, M).
   M = module_resolution.

This indicates the expected behaviour. I could add further answers, and also add the qualifier unexpected, to show what is unexpected. For instance, in total, I could write:

?- module_resolution:get_module(some_predicate, M).
   M = module_resolution.
   M = user, unexpected.

This shows what we expect, and also what we do not expect.

[adri326]

Regarding your comment on #2745 @triska, this does seem to confirm my worry: there currently is no code that is able to retrieve the current context at runtime, and I don't want to introduce more complexity to term/goal expansion by making a custom case.

I guess that making strip_module a meta-predicate is the way to go? As long as I ensure that it continues to behave as expected despite the additional module: prefix.

If I do that then should the unspecified case be replaced with a throw (since that would indicate a case of #2745 or some future issue)? The default_module/1 predicate I introduced in the current solution would disappear as a result.

[triska]

It may well be that strip_module/3 is the wrong abstraction for this purpose!

I think it should be statically decidable to know in which module an unqualified goal is meant to be called, at least as an "approximation" in the sense that the outermost module prefix should be statically deducible: It is either user as the default module, or the module that is being compiled (an "inner" module prefix may still become available dynamically, for instance in: Goal = suprise:hello, Goal, in which nobody knew statically that Goal would be called in surprise, but we do know the context module in which it is being compiled). (Any other opinions?)

So, I think that prolog_load_context/2, which gives us the context module at compilation time, should be enough, at least to fix the original issue (#2725)? Do you have a case in which such static resolution is not sufficient?

[triska]

Or, phrased differently: What if strip_module/3 uses the (static) context module as the default module?

[adri326]

As I have it, it currently essentially calls prolog_load_context already (default_module/1 can be reimplemented in terms of it).

But the question I still have is when prolog_load_context should be called: either during goal expansion (meta predicates can get us 99% of the way there), or when it is evaluated (as it's currently implemented, but most uses of strip_module happen within metapredicates already).

Honestly I wouldn't bikeshed on this too much. I would instead vouch on having good error messages around this subject, like a hint to add the prefix if the predicate is found in a loaded module.

[triska]

The suggestion is that strip_module/3 uses prolog_load_context/2 to acquire the default module. It is evaluated whenever it is used, for example during goal expansion.

[triska]

like a hint to add the prefix if the predicate is found in a loaded module.

To clarify: The purpose of everything here is only to add the prefix of the context module, i.e., the name of the module that is being compiled. That is what every goal within the module implicitly refers to, it is the implicit outermost module qualifier of every goal that occurs in the module.

A meta_predicate/1 declaration makes this qualification explicit for arguments of goals. The goal expansion we are discussing must make the qualification explicit for goals.

[triska]

As an example, let's consider m.pl consisting of:

:- module(m, []).

:- use_module(library(dcgs)).

t --> "hello".

With these definitions, I get:

$ scryer-prolog m.pl
?- m:phrase(t, Cs).
   Cs = "hello".

I would say this works as expected: Note that even though phrase/2 is not itself defined in module m, a call of that predicate in the context of that module is correctly and automatically resolved to use the imported library predicate from library(dcgs). There is no need to mention the dcgs module prefix in the call, only the m: prefix was needed.

[triska]

Specifically, I suggest the following slightly changed definition of strip_module/3 in loader.pl:

strip_module(Goal, M, G) :-
    '$strip_module'(Goal, MQ, G),
    (  MQ = specified(M) ->
       true
    ;  MQ = unspecified,
       load_context(M)
    ).

What do you think?

[adri326]

As I hinted before, my current implementation is very close to doing that already, so I have no issue using some of the already-defined predicates (that I didn't know of a few days ago). The only worry is that load_context fails if we are in the user context, which would in turn make strip_module fail, though that can be addressed within load_context directly.

[triska]

Perfect, thank you a lot! This looks very good!

[adri326]

There is one small change that needs to be done to strip_module/3 (indicated in my TODO), on whichever PR between this one and #2756 gets merged last.

[adri326]

Little bit of cleanup now that #2756 is merged, this PR should now be ready to merge :)