tk3369 / BinaryTraits.jl

Can do or not? It's easy. See https://tk3369.github.io/BinaryTraits.jl/dev/
MIT License
53 stars 3 forks source link

Cross-module interface implementation #39

Closed tk3369 closed 4 years ago

tk3369 commented 4 years ago

Problem statement

Up till now, the trait/assignment/interface/checker functionalities work fine if everything is defined in the same module. It becomes tricky when the trait is defined in one module but needs to be implemented in another. There are a couple of issues:

  1. Namespace - what if the same trait is defined from different third-party modules and they mean different things?

  2. Currently, the traits/prefix/interface maps are stored in BinaryTraits module and it is

Proposed solution

In order to support a module implementing an interface defined from another module, I would like to propose a new syntax. The following hypothetical example illustrate how we could declare our data type for conforming to the Tables.jl's RowTable interface from this fork.

@assign AwesomeTable with  Tables.RowTable

Having implemented PR #38 , it is now possible to implement this syntax change. The macro @assign can just parse the module name (caution: it could have multiple tokens) and then look up the trait from the client module's traits map. In this case, the RowTable trait would be stored in the Tables module's map. When we check the interface contracts, we would have to look up the contracts as defined in Tables module as well.

@KlausC - let me know what you think...

KlausC commented 4 years ago

I see the problem and agree. To check if I understood the solution I just note down my understanding:

  1. The definitions of the @trait and @implement statements are stored in dictionaries local to the modules containing these definitions.
  2. The @assign data are stored locally in the module where the @assign macro is called.
  3. The used dictionaries are read-only for accesses from outside their defining module.
tk3369 commented 4 years ago

Right on! As for #3, the dictionaries are not exported and I don't expect to build any specific read-only barrier.

I haven't thought through all the implementation details yet but here's a rough draft -

When @assign is called in module Y, it may reference a trait defined in a different module X. In the above example, Y.AwesomeTable can implement the contracts set forth by the Tables.RowTable trait. Once assignment is done, the Y.traits_map dictionary would hold the mapping AwesomeTable => Tables.IsRowTable.

Then, the @check function may be called from module Y (as it's in the implementer's interest to make sure that the implementation is correct). The macro would just look up the contracts from Tables and check them against Y.AwesomeTable.

KlausC commented 4 years ago

This test case should work then - I thought.

julia> module X
           using BinaryTraits
           @trait RowTable prefix Is,Not
           @implement IsRowTable by row(::Integer)
       end
Main.X

julia> module Y
           using BinaryTraits
           using ..X
           struct AwesomeTable end
           @assign AwsomeTable with X.RowTable
           @check(AwsomeTable)
           row(::AwsomeTable,::Integer) = 1
           @check(AwsomeTable)
       end
ERROR: LoadError: Bug, no trait has been defined for module Main.Y yet
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] get_prefix_map at .julia/dev/BinaryTraits/src/trait.jl:11 [inlined]
 [3] prefixes(::Module, ::Symbol) at .julia/dev/BinaryTraits/src/trait.jl:20
 [4] can_prefix(::Module, ::Symbol) at .julia/dev/BinaryTraits/src/trait.jl:23
 [5] can_type_symbol(::Module, ::Symbol) at .julia/dev/BinaryTraits/src/trait.jl:26
 [6] @assign(::LineNumberNode, ::Module, ::Symbol, ::Symbol, ::Union{Expr, Symbol}) at .julia/dev/BinaryTraits/src/assignment.jl:70
KlausC commented 4 years ago

I don't expect to build any specific read-only barrier.

I just meant @implement should not be (easily) callable from outside the module defining the trait.

tk3369 commented 4 years ago

The above test does not work because the @assign macro assumes that the trait was defined in the same module and so it tries to look up the prefixes in current module's dictionary. You get the "Bug" message because the dictionary does not even exists given that no trait has been defined in module Y yet.

As for implementation, here's my current thought process:

First, it may be easier if the syntax requires the foreign trait to be prefixed by the module name i.e. X.RowTable rather than just RowTable (even if the implementer has imported the name from X). Because macros only handles syntax, we don't really have any context if only RowTable is mentioned. Tell me if I'm wrong.

Currently, the @assign macro expands as follows https://github.com/tk3369/BinaryTraits.jl/blob/bbc1f2c0573572948f78843723a6c3dced5b1a63/src/assignment.jl#L82

Now, let's say module prefix is required. Then, a statement like @assign AwesomeTable with X.RowTable may be expanded into something like this instead

BinaryTraits.assign(Y, AwesomeTable, BinaryTraits.find_can_type(:X, :RowTable))

Yes, we have to work with symbols because macro only deals with syntax. Given that :X must be an imported symbol in current module Y, we can locate a reference of module X from module Y with a function like this:

function find_imported_module(m::Module, imported_module_name::Symbol)
    syms = names(m, all = true, imported = true)
    if imported_module_name in syms
        return getfield(m, imported_module_name)
    else
        return nothing
    end
end

Then, we can look up the can-type of RowTable from the dictionary in module X. Now that we have found a reference to IsRowTable, it can be assigned and saved into module Y's traits map dictionary __binarytraits_traits_map.

Next, how do we deal with @check macro? I haven't thought about that yet but we will likely have to go through a similar exercise there.

tk3369 commented 4 years ago

Setting to high priority because this use case is very common (especially in the Julia ecosystem). A package developer designs an interface and expect users to implement.

KlausC commented 4 years ago

the @assign macro assumes that the trait was defined in the same module

why this assumption? that contradicts the cross-module implementation IMHO

it may be easier if the syntax requires the foreign trait to be prefixed by the module name i.e. X.RowTable rather than just RowTable

I think that over-complicates the thing. It is possible to pass the Expr to the assign function, as we do also for the type T, which need not be a symbol.

BinaryTraits.assign($mod, $T, $this_can_type) 

should work without change. The assign macro must only accept Union{Symbol,Expr} for the third argument.

The assign macro has no chance to check the existence of the trait; that can be done in the assign function, though. To find the correct module for the lookup we can use parentmodule(can_type)

tk3369 commented 4 years ago

why this assumption? that contradicts the cross-module implementation IMHO

I'm just referring to the current code. It doesn't work across modules yet, and that's why this GitHub issue is filed...

It is possible to pass the Expr to the assign function, as we do also for the type T, which need not be a symbol.

That's true. I'm probably over-complicating the issue. Thanks for the feedback. I will get my hands dirty again this weekend :-)

KlausC commented 4 years ago

I have an additional remark to the @assign macro. It feels a little bit strange, that we assign with the trait name, and not with the specialization. That mean, I would find it clearer if we could do:

@trait Honest prefix Is,Not

@assign Judge with X.IsHonest
@assign Criminal with X.NotHonest

That had the following positive effects to the code quality:

  1. The prefix_map dictionary and related functions are no longer needed. The prefix symbols are only required in the @trait macro, where they are available from the macro arguments.
  2. I think, it is a bad design, if the macro code refers to runtime objects like this dictionary. The macro code should, where possible, restrict to fiddling with the AST without referring to previously generated runtime data.
  3. I think, those macro arguments, which have the syntax of plain Julia code, should not have a special meaning in the context of macro expansion; at least this special purpose should be restricted as far as possible. Examples for 3.:
  4. @assign ... with X.RowTable is bad, because X.RowTable looks like a name in module X, but no such object exists. At the other hand X.IsRowTable exists and can be easily verified.
  5. It should be possible to write TR = X.IsRowTable; @assign ... with TR and that that should have the same effect as @assign ... with X.IsRowTable. That would be in line with a general principle in Julia, we can assign any type of object variables.