Improve Javadoc and API succinctness using more package-private classes

[rheitjoh]

@JanBobolz suggested we improve the javadoc as some parts of it are fairly messy due too basically every class having public visibility.
His suggestion was to compile the javadoc with -public and --show-members protected flags, as well as mark classes we do not want to be part of the public API as package-private.
protected methods and member variables would then still be shown.

The only allowed class access modifiers are public and package-private.
By marking a class as package-private, it is only visible inside its package.

This has the advantage of allowing for more backwards-compatible API changes, i.e. reduced likelihood of major/minor version increases being necessary, as well as reducing noise inside the rendered Javadoc.

What to mark as package-private

The classes that I have selected as being likely candidates for a package-private access modifier are as follows:

Classes that implement RepresentationHandler

These are ArrayRepresentationHandler, DependentRepresentationHandler, ListAndSetRepresentationHandler, MapRepresentationHandler, StandaloneRepresentationHandler.

These classes are only used inside ReprUtil and will most likely never be used anywhere else, unless someone would want to implement their own ReprUtil (unlikely).

The serialization.annotations.internal package would then have to be merged with serialization.annotations to allow ReprUtil continued access.

Would then also make sense to make the RepresentationHandler interface itself package-private.
I think if you were to ever write your own representation handler, you would do it as part of the Math library and not in your own project.
Especially since there is no API to add new representation handlers to ReprUtil from the outside.
So would need to edit ReprUtil in any case.

Counting Impl classes

The user should always use the non-Impl version of the counting classes.
The non-Impl versions all make use of lazy group stuff to allow for counting multi-exponentiations as well as count in a way that results in the least amount of operations (since optimizations can be done once compute() is called).

The Impl classes just allow you to count via the basic group stuff.
So if you are replacing a BasicGroup with the counting group, this would lead to more accurate counting.
However, the basic group stuff should only be used for already very efficient groups.
So why even bother with counting group operations in that case?
They don't really matter for performance.
Plus you can emulate a basic group implementation by calling compute() after every call.

So would make CountingBilinearGroupImpl, CountingBilinearMapImpl, CountingGroupElementImpl, CountingGroupImpl, CountingHomomorphismImpl, and HashIntoCountingGroupImpl package-private.

Supersingular classes

Would make everything but SupersingularBilinearGroup package-private.
Then one can still use the supersingular bilinear group as usual.
Not possible to wrap SupersingularBilinearGroup in a BasicBilinearGroup anymore, but we can add a class for that if we think that is useful.

Barreto-Naehrig classes

Would make everything but BarretoNaehrigBilinearGroup and BarretoNaehrigParameterSpec package-private.
Same reasoning as for supersingular classes.

Lazy group classes

Theoretically one could make some classes package-private, like ExpLazyGroupElement.
It seems a bit random though as others, e.g. ConstLazyGroupElement are used by the counting group classes and therefore could not be made package-private.

Ring group classes

Theoretically, one could make RingGroupImpl, RingAdditiveGroupImpl, and RingUnitGroupImpl package-private.
Only RingGroup is really needed as a public API as you can get the additive and unit subgroups via additiveGroupOf() and unitGroupOf().

Would mean, since RingGroup is a BasicGroup, that you cannot use the lazy group stuff with the ring group stuff, but are there any ring groups where lazy evaluation is interesting?
If there were, we could just introduce a RingLazyGroup anyways.

An issue would be that you could not extend RingAdditiveGroupImpl or RingUnitGroupImpl for your specific ring, which would allow you to override estimateCostInvPerOp with a more accurate number.
Not sure there any other reasons to want to extend those classes.

Mcl bil group classes

Basically the same thing as for the other bil pairings, everything but MclBilinearGroup.

[JanBobolz]

It seems a bit random though as others, e.g. ConstLazyGroupElement are used by the counting group classes and therefore could not be made package-private.

We could always make LazyGroup::wrap public, instead. I'm in favor of hiding the subclasses of the LazyGroup, they are very much implementation detail stuff that nobody should use.

An issue would be that you could not extend RingAdditiveGroupImpl or RingUnitGroupImpl for your specific ring, which would allow you to override estimateCostInvPerOp with a more accurate number.

Probably the estimateCostInvPerOp() method should then become part of the Ring interface.

Good work! I agree with your assessments.

[rheitjoh]

Probably the estimateCostInvPerOp() method should then become part of the Ring interface.

So we would then delegate the estimateCostInvPerOp() calculation to the ring attribute? We could add a Boolean argument that allows the caller to indicate whether they are interested in the additive group's or multiplicative group's cost. Then we could still have different costs for those. Seems like a reasonable approach to me (also more sensible than expecting the user to override the ring group impl classes).

[JanBobolz]

Why boolean argument instead of separate estimateCostInvPerOp and estimateCostNegPerOp methods? (I'm okay with either, but wondering why you'd go with the boolean)

[rheitjoh]

No real reason, just what came to mind first. Two different methods do seem more intuitive though, so I like your suggestion more.

[rheitjoh]

An issue came up while making the supersingular classes package-private:

The StructureStandaloneReprTest cannot find the Impl classes anymore since they are package-private (and the repr tests are not in the same package). My suggestion here would be to just remove those repr tests since testing SupersingularBilinearGroup itself should already cover the serialization code of the Impl classes. To make that work, we additionally need to tell the StandaloneReprTest to not collect those classes; else it complains that they are missing tests. I suppose we could just tell the test to only collect public classes.

I have also made a change to RepresentableRepresentation#recreateRepresentable in that the obtained constructor is explicitly set to be accessible; otherwise recreating package-private classes does not work since it cannot find the constructor.

Also, the GroupImplTests faces similar problems with not accessible classes. Similarly, I would suggest removing those Impl tests and just relying on the direct GroupTests. If we wrap all those Impl classes in BasicGroup (or even also LazyGroup) we can still do the same tests.

[rheitjoh]

It seems a bit random though as others, e.g. ConstLazyGroupElement are used by the counting group classes and therefore could not be made package-private.

We could always make LazyGroup::wrap public, instead. I'm in favor of hiding the subclasses of the LazyGroup, they are very much implementation detail stuff that nobody should use.

Ok, so still public need to be LazyGroup, LazyBilinearMap, LazyBilinearGroup, HashIntoLazyGroup, LazyGroupElement, LazyGroupHomomorphism.

[rheitjoh]

Ok, so more issues have come up. ExpTests and GroupTests both rely on debug group impl classes, which are no longer accessible. Generally, tests that use any impl classes will have problems since no impl classes are publicly available anymore (unless they are in the right package).
With the debug / counting classes you have no choice of whether to wrap them in a basic or lazy group, and the proper pairing groups are probably a bit too slow to use them in tests.

For now I will just be using reflection to get back access to the impl classes where needed in those tests.

[JanBobolz]

Maybe that's an indicator that the GroupImpl stuff is actually public API. Not sure. But the issues we have now will be had by others, too. Want to implement a new Group (super-lazy or something) and want to test it? Tough luck, none of the GroupImpls are accessible.

I'm thinking maybe the following scenario is not too bad:

• We make GroupImpls public.
• For implementors of new GroupImpls: We keep the GroupImpl tests (generally, the GroupImplTest could also test stuff that becomes inaccessible through the BasicGroup).
• For implementors of new Groups: They can now test their implemenentation with existing GroupImpls.
• We allow people to use their own Groups (in "production") with, for example, mcl, if they just want to optimize some multiexponentiation or something.
• We add Javadoc to the BasicGroups that advises people not to use them directly but to use the non-impl version instead or wrap it into a Basic/LazyGroup.

The disadvantage is that we're slightly less free in changing stuff in the GroupImpls. But maybe that's healthy. Keep a nice generic separating interface between Group and Impl so that every Group can use every Impl.

What do you think?

[rheitjoh]

Your argument for GroupImpl implementations can also be formulated for HashIntoGroupImpl, GroupHomomorphismImpl, BilinearGroupImpl and BilinearMapImpl implementations. So it seems that one should make all those public as well, which largely negates the intended Javadoc cleanup.

However, if I imagine I am implementing some cryptographic scheme and I need a bilinear group. I should be consulting the documentation page, and not the javadoc. The documentation will tell me which class I am interested in. So I can then consult the javadoc for that class to gain information about its API. Ideally I won't just be scrolling through the javadoc. Of course, this hinges on the quality of the documentation. However, I haven't used the rendered javadoc even once, so maybe my opinion is not of much use here.

Overall though, I think for the classes where it does not pose such problems as DebugGroupImpl we can still make them package-private. Perhaps just have the debug classes as an exception (still public) that can be used for testing? The only problem is that then you can only use the debug group stuff with your new group implementation. Otherwise you have to implement subclasses like we do currently for e.g. BarretoNaehrigBilinearGroup (and they have to be part of that Math package). In general though: Are we that concerned with users implementing new Groups? I doubt that use case will ever appear.

[JanBobolz]

Perhaps just have the debug classes as an exception (still public) that can be used for testing?

I like that idea.

I doubt that use case will ever appear.

If it does, we can always make classes public again.