JDK-8150964 : j.l.i.MethodHandles.whileLoop(...) small documentation issues
  • Type: Sub-task
  • Component: core-libs
  • Sub-Component: java.lang.invoke
  • Affected Version: 9
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2016-03-01
  • Updated: 2016-04-08
  • Resolved: 2016-04-08
Related Reports
Blocks :  
Description
Please, observe j.l.i.MethodHandles.whileLoop(...)  sections:

1. Returns: the value of the loop variable as the loop terminates.
This could mislead users because actually this method returns method handle representing loop. But this information is necessary to, I suppose it could be good to store this assertion somewhere in documentation.
2. Throws: IllegalArgumentException - if any argument has a type inconsistent with the loop structure
It looks a little bit unclear, because there is no definition for 'loop structure'. 
3. The pred handle must have an additional leading parameter of the same type as init's result, and so must the body.
-- It looks a little bit unclear, it's hard to understand what kind of parameters should 'pred' and body' have. It's clear that leading parameter is the same as 'init' return type, but there is no rules for other parameters. It could be deducted from pseudocode, but maybe it would be good to make this description clearer. 
-- The parameter list of resulting method handle could be deducted from pseudocode too, but it couldn't be determined from preceeding documentation.
4. The implementation of this method is equivalent to: 
 MethodHandle whileLoop(MethodHandle init, MethodHandle pred, MethodHandle body) {
     MethodHandle[]
         checkExit = {null, null, pred, identity(init.type().returnType())},
         varBody = {init, body};
     return loop(checkExit, varBody);
 }


I suppose we can't say they are equals because according to doc, 'init'=null should work correctly, but according to given documentation, 'init'=null will lead to NullPointerException (see init.type()). So same calls with init==null for jdk implementation and for given implementation have different results. Example attached.
Comments
Thanks, [~asolodkaya]. Addressed internally; fix will be pushed with JDK-8151179.
08-04-2016

It seems these special cases in whileLoop/loop are clear and pred/body docs are clear too. I will review loop(...) again during development of the deeper tests, now it looks clear.
07-04-2016

Thanks, [~asolodkaya]. I've documented the special case on MHs.loop as follows: * <em>Special case.</em> As a consequence of step 1A above, the {@code loop} combinator has the following property: * <ul> * <li>Given {@code N} clauses {@code Cn = {null, Pn, Sn}} with {@code n = 1..N} and predicates {@code Pn} that are * either {@code null} or have no parameters (only one {@code Pn} has to be non-{@code null}). * <li>Given step handles {@code Sn} with signatures {@code (A1..AX)Rn}. * <li>If {@code Rn == An} for {@code n = 1..Q} with {@code Q <= X}, the parameter types {@code A1..AQ} will be * interpreted as loop-local state elements, and the remaining parameter types {@code AQ+1..AX} will determine the * resulting loop handle's parameter types. * </ul> And on MHs.whileLoop/doWhileLoop as follows: * <em>Special case.</em> The special case documented for {@link #loop(MethodHandle[]...)} manifests for this method * as follows. If {@code init} is not given and {@code pred} has no parameter types, and if {@code body} has a * signature of the form {@code (AB...)A} (where {@code A} denotes a type, and {@code B...} an optional sequence of * types), the {@code A} argument of {@code body} will be interpreted as loop-local state, and {@code B...} will * determine the resulting loop handle's signature. The attached file WL.java carries some examples. I've addressed the minor documentation remarks accordingly. The final issue about pred/body parameter lists' relation to init's should be clarified by the updated documentation, viz. * The {@code pred} and {@code body} handles must have effectively identical parameter type lists (see the * documentation for {@link #loop(MethodHandle[]...)} for a definition). In case the {@code init} handle is not * {@code null}, the {@code pred} and {@code body} handles must also have an additional leading parameter of the * same type as {@code init}'s result, and their parameter type lists after the additional leading parameter must be * a prefix of the one of {@code init}. The additional leading parameter can be omitted if there are no other * parameters. If {@code init} is {@code null}, implying the loop handle's result type is {@code void}, the longer * of {@code pred} and {@code body}'s parameter type lists determines the resulting loop handle's parameter type * list.
05-04-2016

With regard to previous comment, I suppose, from one hand that 'documenting' the case will lead to complicated docs and complicated usage, so it is much more easier to prohibit. From another hand it is logically enough to have this ability. In the new doc: >> The parameter type list of init will also be that of the resulting handle; unless init is null, in which case the resulting handle's return type will be void, and its parameter type list will be determined from the pred handle (see below). I suppose it would be good to mention 'body' along with 'pred' here. Information below shows that pred/body determines parameter type list. >> The pred and body handles must have effectively identical parameter type lists. It would be good to point to the loop(..) here (to mention place there you can find definition of 'effectively identical') >> In case the init handle is not null, the pred and body handles must also have an additional leading parameter of the same type as init's result. I've found that implementation permits to omit this leading parameter (in the case then all enxt parameters are omitted too, of course). Please, see WhileLoopProposal5.java for details (1, 2 example) The case is not covered by new documentation version: - init is not null and not void - 'body' and 'pred' have effectively identical parameter type list - but "parameter_list(body) minus leading_argument" is not effectively identical to 'init' parameter list - body have arguments (of course, it means that body have at least two arguments, because one argument => previous condition is always false) Examples 3/4 in WhileLoopProposal5 illustrate this case for body and pred respectively. Shortly speaking, there is no rule like " body's and pred's parameter type list after loop variable should be 1. effectively identical to 'init' parameter type list 2. shorter then 'init' parameter list ", so now it is permitted to have 3/4 example combinations. I have no idea how to describe it clearer, I hope you understand that I mean.
04-04-2016

Take S to mean String.class, and O to mean Object.class. If you pass a MH combination such as a ()Z for pred and (SO)S for step in a clause like {null, step, pred, null} to MHs.loop(), the same behaviour is observable: the leading parameter type of step will be considered as a local state argument if step has the same return type. This likely applies to the one-clause case for MHs.loop(), as interfering with arguments in other positions will violate effective identity of parameter type lists. So yes, document or prohibit.
04-04-2016

>> * Body not returning void, first parameter turning into local state... I'm sorry, I didn't catch what do you mean. Which options do we have? I assume we have two options: document it or prohibit it (throw IAE from whileLoop(...), not from loop(...) ). Would you like to propose another options?
04-04-2016

FTR, here's the updated documentation for whileLoop. /** * Constructs a {@code while} loop from an initializer, a body, and a predicate. This is a convenience wrapper for * the {@linkplain #loop(MethodHandle[][]) generic loop combinator}. * <p> * The loop handle's result type is the same as the sole loop variable's, i.e., the result type of {@code init} and * {@code body} (these must have the same result type, while {@code pred} must have {@code boolean} as its result * type). The parameter type list of {@code init} will also be that of the resulting handle; unless {@code init} is * {@code null}, in which case the resulting handle's return type will be {@code void}, and its parameter type list * will be determined from the {@code pred} handle (see below). * <p> * The {@code pred} and {@code body} handles must have effectively identical parameter type lists. In case the * {@code init} handle is not {@code null}, the {@code pred} and {@code body} handles must also have an additional * leading parameter of the same type as {@code init}'s result. If {@code init} is {@code null}, implying the loop * handle's result type is {@code void}, the longer of {@code pred} and {@code body}'s parameter type lists * determines the resulting loop handle's parameter type list. * <p> * These constraints follow directly from those described for the {@linkplain MethodHandles#loop(MethodHandle[][]) * generic loop combinator}. * <p> * Here is pseudocode for the resulting loop handle. In the code, {@code V}/{@code v} represent the type / value of * the sole loop variable as well as the result type of the loop; and {@code A}/{@code a}, that of the argument * passed to the loop. * <blockquote><pre>{@code * V init(A); * boolean pred(V, A); * V body(V, A); * V whileLoop(A a) { * V v = init(a); * while (pred(v, a)) { * v = body(v, a); * } * return v; * } * }</pre></blockquote> * <p> * @apiNote Example: * <blockquote><pre>{@code * // implement the zip function for lists as a loop handle * static List<String> initZip(Iterator<String> a, Iterator<String> b) { return new ArrayList<>(); } * static boolean zipPred(List<String> zip, Iterator<String> a, Iterator<String> b) { return a.hasNext() && b.hasNext(); } * static List<String> zipStep(List<String> zip, Iterator<String> a, Iterator<String> b) { * zip.add(a.next()); * zip.add(b.next()); * return zip; * } * // assume MH_initZip, MH_zipPred, and MH_zipStep are handles to the above methods * MethodHandle loop = MethodHandles.whileLoop(MH_initZip, MH_zipPred, MH_zipStep); * List<String> a = Arrays.asList("a", "b", "c", "d"); * List<String> b = Arrays.asList("e", "f", "g", "h"); * List<String> zipped = Arrays.asList("a", "e", "b", "f", "c", "g", "d", "h"); * assertEquals(zipped, (List<String>) loop.invoke(a.iterator(), b.iterator())); * }</pre></blockquote> * * <p> * @implSpec The implementation of this method is equivalent to (excluding error handling): * <blockquote><pre>{@code * MethodHandle whileLoop(MethodHandle init, MethodHandle pred, MethodHandle body) { * MethodHandle[] * checkExit = {null, null, pred, init == null ? zero(void.class) : identity(init.type().returnType())}, * varBody = {init, body}; * return loop(checkExit, varBody); * } * }</pre></blockquote> * * @param init initializer: it should provide the initial value of the loop variable. This controls the loop's * result type. Passing {@code null} or a {@code void} init function will make the loop's result type * {@code void}. * @param pred condition for the loop, which may not be {@code null}. * @param body body of the loop, which may not be {@code null}. * * @return a method handle embodying the looping behavior as defined by the arguments. * @throws IllegalArgumentException if any argument has a type inconsistent with the above requirements. * * @see MethodHandles#loop(MethodHandle[][]) * @since 9 */
04-04-2016

Thanks for the feedback, [~asolodkaya]! Addressing your points in the order they appear in in the comments above: * Body should be allowed to be null, like in JDK-8151026: this is fixed in the changeset for that issue. * Add information about return types: yes. * @implSpec code equivalence minus error handling, like for JDK-8151903: good point, I'll address this. * Pred and body parameter type lists can be effectively identical: correct; and in case init is null, the longer of them determines the resulting parameter type list. * Body not returning void, first parameter turning into local state: this is a consequence of step 1A in MethodHandles.loop(). If init is omitted, the return type of step determines the type of one local state variable IFF the parameter type of step at the position corresponding to the clause is the same as its return type. This may be a bit complicated to use and seems to require some more documentation on MethodHandles.loop() as well as tests. Do you agree?
04-04-2016

In the case init is null, body have the same behaviour as mentioned in https://bugs.openjdk.java.net/browse/JDK-8151736?focusedCommentId=13919991&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13919991 So if first parameter of body = return type of body and return type of body is not void, then this parameter becames 'local state' and it is updated after each body call. Please, observe WhileLoopProposal2.java and WhileLoopProposal3.java, for details.
31-03-2016

>> @param body body of the loop, which may not be {@code null}. According to JDK-8151026 comments, body should be allowed to be null New version of spec covers parameter list restrictions completely. It would be good to add information about necessary body/pred return types. And it would be good to add "The implementation of this method is equivalent to (excluding error handling): " to implementation notes the same way it was done for JDK-8151903
31-03-2016

>> unless init is null, in which case the resulting handle's return type will be void, and its parameter type list will be determined from the pred handle (see below). >> If init is null, implying the loop handle's result type is void, the parameter type lists of pred and body must be identical and determine the resulting loop handle's parameter type list. In current implementation, body and pred are permitted to have effectively identical parameter lists. Please, see WhileLoopProposal0.java and WhileLoopProposal1.java for details: pred = (String, List)Z, body = (String, List, Object)V -> (String,List,Object)V pred = (String, List, Object)Z, body = (String, List)V -> (String,List,Object)V pred = (String)Z, body = (String, Object)V -> (String,Object)V pred = (String, Object)Z, body = (String)V -> (String,Object)V pred = ()Z, body = (String, Object)V -> (String,Object)V pred = (String, Object)Z, body = ()V -> (String,Object)V pred = ()Z, body = (String)V -> (String)V pred = (String)Z, body = ()V -> (String)V
31-03-2016

[~asolodkaya], please comment on this proposal for addressing the documentation issues (only the doc for whileLoop is shown; that for doWhileLoop is changed accordingly): /** * Constructs a {@code while} loop from an initializer, a body, and a predicate. This is a convenience wrapper for * the {@linkplain #loop(MethodHandle[][]) generic loop combinator}. * <p> * The loop handle's result type is the same as the sole loop variable's, i.e., the result type of {@code init}. * The parameter type list of {@code init} will also be that of the resulting handle; unless {@code init} is * {@code null}, in which case the resulting handle's return type will be {@code void}, and its parameter type list * will be determined from the {@code pred} handle (see below). * <p> * In case the {@code init} handle is not {@code null}, the {@code pred} and {@code body} handles must have the same * parameter list as {@code init}, plus an additional leading parameter of the same type as {@code init}'s result. * If {@code init} is {@code null}, implying the loop handle's result type is {@code void}, the parameter type lists * of {@code pred} and {@code body} must be identical and determine the resulting loop handle's parameter type list. * <p> * These constraints follow directly from those described for the {@linkplain MethodHandles#loop(MethodHandle[][]) * generic loop combinator}. * <p> * Here is pseudocode for the resulting loop handle. In the code, {@code V}/{@code v} represent the type / value of * the sole loop variable as well as the result type of the loop; and {@code A}/{@code a}, that of the argument * passed to the loop. * <blockquote><pre>{@code * V init(A); * boolean pred(V, A); * V body(V, A); * V whileLoop(A a) { * V v = init(a); * while (pred(v, a)) { * v = body(v, a); * } * return v; * } * }</pre></blockquote> * <p> * @apiNote Example: * <blockquote><pre>{@code * // implement the zip function for lists as a loop handle * static List<String> initZip(Iterator<String> a, Iterator<String> b) { return new ArrayList<>(); } * static boolean zipPred(List<String> zip, Iterator<String> a, Iterator<String> b) { return a.hasNext() && b.hasNext(); } * static List<String> zipStep(List<String> zip, Iterator<String> a, Iterator<String> b) { * zip.add(a.next()); * zip.add(b.next()); * return zip; * } * // assume MH_initZip, MH_zipPred, and MH_zipStep are handles to the above methods * MethodHandle loop = MethodHandles.whileLoop(MH_initZip, MH_zipPred, MH_zipStep); * List<String> a = Arrays.asList("a", "b", "c", "d"); * List<String> b = Arrays.asList("e", "f", "g", "h"); * List<String> zipped = Arrays.asList("a", "e", "b", "f", "c", "g", "d", "h"); * assertEquals(zipped, (List<String>) loop.invoke(a.iterator(), b.iterator())); * }</pre></blockquote> * * <p> * @implSpec The implementation of this method is equivalent to: * <blockquote><pre>{@code * MethodHandle whileLoop(MethodHandle init, MethodHandle pred, MethodHandle body) { * MethodHandle[] * checkExit = {null, null, pred, init == null ? zero(void.class) : identity(init.type().returnType())}, * varBody = {init, body}; * return loop(checkExit, varBody); * } * }</pre></blockquote> * * @param init initializer: it should provide the initial value of the loop variable. This controls the loop's * result type. Passing {@code null} or a {@code void} init function will make the loop's result type * {@code void}. * @param pred condition for the loop, which may not be {@code null}. * @param body body of the loop, which may not be {@code null}. * * @return a method handle embodying the looping behavior as defined by the arguments. * @throws IllegalArgumentException if any argument has a type inconsistent with the above requirements. * * @see MethodHandles#loop(MethodHandle[][]) * @since 9 */ Note that a small fix in the implementation is required when the zero method is introduced with JDK-8150829; the documentation change for the @implSpec tag already reflects this. I'm holding off on marking this as internally resolved until the patch can be applied against that.
22-03-2016