derive4j / hkt

Higher Kinded Type machinery for Java
BSD 3-Clause "New" or "Revised" License
81 stars 9 forks source link

generation of narrow method #8

Closed jbgi closed 7 years ago

jbgi commented 8 years ago

We should check that a static 'narrow' method with correct signature (and implementation if possible) is present in the class (unless annotated @Data). The method could have another name. (or should it be normalized ?)

gneuvill commented 8 years ago

I have two questions on that matter :

1- are we absolutely sure that given

  class Thing<A> implements __<Thing.µ, A> {
    static <A> Thing<A> coerce(__<Thing.µ, A> thing) {...}
  }

then the only possible implementation of coerce is a cast ? (ignoring null and runtime exceptions of course) By virtue of parametricity, I would tend to answer positively, but I'm not sure...

2- the more I think about it, the more I am inclined to favour the generation of the coercion methods by this very project, for the following reasons :

What do you think ?

Example of Coerce class :

  public final class Coerce {
    private Coerce() {}

    public static <A> Maybe<A> maybe(__<Maybe.µ, A> maybe) {...}

    public static <A> List<A> list(__<List.µ, A> list) {...}

    // etc...
  }
jbgi commented 8 years ago

the only possible implementation of coerce is a cast ?

in the case of phantom types, at least, this is not the only implementation:

class PhantomInt<A> implements __<PhantomInt<?>, A> {
  int i;
  PhantomInt(int i) {
    this.i = i;
  }
  static <A> PhantomInt<A> maliciousCoerce(__<PhantomInt<?>, A> pi) {
    return new PhantomInt<>(new Random().nextInt());
  }
}

thus, the importance of generating or verifying the implementation.

gneuvill commented 8 years ago

Aha ! I should have said : "ignoring null and runtime exceptions and side effects of course", cause your example relies on a side-effect, doesn't it ? So that seems to validate the fact that a cast is the only legitimate implementation for a method with a 'coercing' signature ?

thus, the importance of generating or verifying the implementation.

All the more reasons to consider the generation of coercion methods the last step or better, the consequence of the type checking , don't you think ? (and thus to do it right here, in the @derive4j/hkt project...?)

UPDATE : Oops, I misunderstood you example by focusing on the Random().nexInt() part... (that being said, new PhantomInt is kind of a side effect also, isn't it ?). But that doesn't seem to invalidate the rest of my reasoning... or does it ? Is the maliciousCoerce method really malicious or, in the end, legit ?

jbgi commented 8 years ago

ignoring null and runtime exceptions and side effects of course

well, in my example, returning always return new PhantomInt<>(-1); would also work.

As for generating the coerce methods by hkt, there is going to be some obstacles...

we could generate the methods in a one and only class

This will only works for public classes or if all classes are in the same package. Local classes are another beast (generating a static method for them is not even possible). Also, choosing the right package to put the Coerce class into may not be obvious.

There is also some projects who would want to avoid code generation altogether.

So I think, code generation by hkt processor should be opt-in only, probably through another artifact, "hkt-with-code-gen", with its own annotation processor (sharing code with the normal processor that could disable itself if it detects the code-generating processor)

gneuvill commented 8 years ago

well, in my example, returning always return new PhantomInt<>(-1); would also work.

Yep. But then the pi parameter is ignored. Thus my question : in the case of a static method having a 'coercing signature', is this legitimate ? If not, we don't need to check any implementation, just forbid the signature altogether.

This will only works for public classes or if all classes are in the same package.

This is unfortunate ; but a valid limitation. If you need a private inner class to be an hkt, then write the coercing method yourself. That should not be the majority of cases, should it ?

Local classes are another beast

Must we really take them into account at all ? Do you declare datatypes inside functions in haskell ?

Also, choosing the right package to put the Coerce class into may not be obvious

Configuration ; fallback on the package of the first implementation of __ encountered by default. Not sure it is that big a deal since all the methods in Coerce will be public static...?

There is also some projects who would want to avoid code generation altogether.

Sure. Configuration ?

So I think, code generation by hkt processor should be opt-in only, probably through another artifact, "hkt-with-code-gen", with its own annotation processor (sharing code with the normal processor that could disable itself if it detects the code-generating processor)

Bof. Sounds complicated. To me, the coercing methods generation is either here, or in derive4j. Yet another artifact seems like an unnecessary burden on users...

jbgi commented 8 years ago

Sure. Configuration ?

So, yeah, if it is opt-in via configuration, let's do it! We will also need to take incremental compilation: ie. handling partial rewrite of the Coerce class.

But I'm also bit concerned that, in a given project, one might end-up with a few different Coerce classes from various dependencies: it could lead to some confusion.

As for local classes: writing a static coerce is not possible, so we might want to inspect the code and checks that the cast is done correctly. Local classes are not a very common use case I agree.

gneuvill commented 8 years ago

We will also need to take incremental compilation: ie. handling partial rewrite of the Coerce class.

Can't we just erase and recreate the file altogether ? How do you proceed in derive4j ?

But I'm also bit concerned that, in a given project, one might end-up with a few different Coerce classes from various dependencies: it could lead to some confusion.

Well, I'm temped to rely on configuration once again (i.e the name of the generated class would be configurable)

As for local classes: writing a static coerce is not possible, so we might want to inspect the code and checks that the cast is done correctly. Local classes are not a very common use case I agree.

Yep. I'm on the verge of creating an issue to forbid the implementation of __ interfaces by local classes : would you agree ?

jbgi commented 8 years ago

Can't we just erase and recreate the file altogether ? How do you proceed in derive4j ?

Not if there is a single class with all coerce methods for all hkt classes. in derive4j we can overwite because the generated classes are induced by a single classes. So before overwriting we will need to read existing signature, filter out deleted classes and regenerate.

The other solution is to generate the coerce method in a utility class dedicated to each hkt classes.

jbgi commented 8 years ago

Yep. I'm on the verge of creating an issue to forbid the implementation of __ interfaces by local classes : would you agree ?

I agree, this will be the safest, let's aim for 100% type-safety!

jbgi commented 8 years ago

Also, about the default name of the coerce methods, I was thinking of hktAsX (or maybe narrowToX) where X is the name of the class. I don't really like to put coerce in name as it kind of imply that it is "unsafe". Also X must appear in the name, to avoid erasure clashes and allow non-conflicting static imports.

WDYT?

gneuvill commented 8 years ago

in derive4j we can overwite because the generated classes are induced by a single classes

Aha. Thanks for pointing out incremental compilation, I wouldn't have thought about this. Does this complicate the generation to the point of making it undesirable ? (i.e rely on derive4j only to do it)

let's aim for 100% type-safety!

Music to my hears... That's why I'd really like to have your opinion about my very first comment in https://github.com/derive4j/hkt/issues/8#issuecomment-217280056 (from "Yep." to "...altogether")

I don't really like to put coerce in name as it kind of imply that it is "unsafe".

In my example, only the generated class would be called 'Coerce'. We could name it 'Narrow' by default if you prefer.

Also X must appear in the name, to avoid erasure clashes and allow non-conflicting static imports.

Yep. In fact, the idea I tried to expose with my example was to promote an only qualified usage of the methods (as in Narrow.maybe(myHkt)). This reads fine and is also handy and clear when used by method reference (Narrow::maybe).

jbgi commented 8 years ago

Does this complicate the generation to the point of making it undesirable ?

There can corner cases I am not thinking of indeed. Should be doable though.

Yep. But then the pi parameter is ignored. Thus my question : in the case of a static method having a 'coercing signature', is this legitimate ? If not, we don't need to check any implementation, just forbid the signature altogether.

I now better understand the question: indeed, if we provide the coerce method via code-gen then the user should not have to write it and thus it could be forbidden. On the other hand it is an unsafe cast like any other and we may simply skip looking for it, compiler should warn about it already.

Narrow.maybe(myHkt) is nice indeed.

Thinking out loud on other practical aspects: I think the hkt processor should generate one Narrow class per package, and an annotation in the package-infoclass could be used for configuration: name of class, template for narrow methods, visibility of the generated Narrow class. WDYT?

gneuvill commented 8 years ago

indeed, if we provide the coerce method via code-gen then the user should not have to write it and thus it could be forbidden

That was my point indeed : sorry if I've been confused about it. And I think this is crucial would we want to reach, let's say, 80% type safety (100% would mean check every statements and expressions in the source for illegitimate casts - and forbid null while we're at it) because I don't think we will mistakenly forbid any valid implementation.

On the other hand it is an unsafe cast like any other and we may simply skip looking for it, compiler should warn about it already

The thing is, and I don't know the rationale for this, javac does not emit any warning regarding our coercing casts (don't know if it's the case for all downcasts). But I guess what you're saying is we're in java land wherein forbidding a method signature doesn't prevent the user from resorting to a bunch of nonsensical artefacts... So the question become : when do we stop fighting java ? Where do we put the bar ?

gneuvill commented 8 years ago

Narrow.maybe(myHkt) is nice indeed.

Another variation : Hkt.asMaybe(myHkt) and Hkt::asMaybe