RFR [10] 8194554: filterArguments runs multiple filters in the wrong order

[Paul Sandoz]

I am supportive of this change (the risk to impacting order-dependent stateful MH filter code is smaller than the risk of hitting a string concatenation bug). (We erred on the side of this being a bug and not being a spec change given the pseudo-code in the Java doc.)

IIRC this will require a nod of approval from the project lead.

Paul.

> On Apr 9, 2018, at 2:52 AM, Aleksey Shipilev <shade at redhat.com> wrote:
> 
> Ping.
> 
> -Aleksey
> 
> On 04/05/2018 05:33 PM, Aleksey Shipilev wrote:
>> Hi,
>> 
>> Please review this jdk10 backport.
>> 
>> Original JDK 11 bug:
>>  https://bugs.openjdk.java.net/browse/JDK-8194554
>> 
>> Original JDK 11 fix:
>>  http://hg.openjdk.java.net/jdk/jdk/rev/050352ed64d5
>> 
>> Please note the discussion in JBS comments about the issue. It seems to include the more verbose
>> specification text, and Rob says it makes the patch not directly backportable. Therefore, requesting
>> to backport without the Javadoc change. I just dropped that single line from the original changeset:
>> 
>> 
>> 8194554: filterArguments runs multiple filters in the wrong order
>> Reviewed-by: psandoz, jrose
>> 
>> diff -r b09e56145e11 -r ab2dc3096cdb src/java.base/share/classes/java/lang/invoke/MethodHandles.java
>> --- a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java	Thu Mar 08 04:23:31 2018 +0000
>> +++ b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java	Wed Jan 17 15:17:50 2018 -0800
>> @@ -3836,11 +3836,12 @@
>>     MethodHandle filterArguments(MethodHandle target, int pos, MethodHandle... filters) {
>>         filterArgumentsCheckArity(target, pos, filters);
>>         MethodHandle adapter = target;
>> -        int curPos = pos-1;  // pre-incremented
>> -        for (MethodHandle filter : filters) {
>> -            curPos += 1;
>> +        // process filters in reverse order so that the invocation of
>> +        // the resulting adapter will invoke the filters in left-to-right order
>> +        for (int i = filters.length - 1; i >= 0; --i) {
>> +            MethodHandle filter = filters[i];
>>             if (filter == null)  continue;  // ignore null elements of filters
>> -            adapter = filterArgument(adapter, curPos, filter);
>> +            adapter = filterArgument(adapter, pos + i, filter);
>>         }
>>         return adapter;
>>     }
>> diff -r b09e56145e11 -r ab2dc3096cdb test/jdk/java/lang/invoke/FilterArgumentsTest.java
>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>> +++ b/test/jdk/java/lang/invoke/FilterArgumentsTest.java	Wed Jan 17 15:17:50 2018 -0800
>> @@ -0,0 +1,132 @@
>> +/*
>> + * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
>> + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>> + *
>> + * This code is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 only, as
>> + * published by the Free Software Foundation.
>> + *
>> + * This code is distributed in the hope that it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
>> + * version 2 for more details (a copy is included in the LICENSE file that
>> + * accompanied this code).
>> + *
>> + * You should have received a copy of the GNU General Public License version
>> + * 2 along with this work; if not, write to the Free Software Foundation,
>> + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>> + *
>> + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
>> + * or visit www.oracle.com if you need additional information or have any
>> + * questions.
>> + */
>> +
>> +/*
>> + * @test
>> + * @bug 8194554
>> + * @run testng/othervm test.java.lang.invoke.FilterArgumentsTest
>> + */
>> +
>> +package test.java.lang.invoke;
>> +
>> +import java.lang.invoke.MethodHandle;
>> +import java.lang.invoke.MethodHandles;
>> +import java.util.ArrayList;
>> +import java.util.List;
>> +
>> +import static java.lang.invoke.MethodHandles.*;
>> +import static java.lang.invoke.MethodType.methodType;
>> +
>> +import org.testng.annotations.*;
>> +import static org.testng.Assert.*;
>> +
>> +public class FilterArgumentsTest {
>> +
>> +    @Test
>> +    public static void testFilterA_B_C() throws Throwable {
>> +        FilterTest test = new FilterTest(
>> +            filterArguments(MH_TEST, 0, MH_FILTER_A, MH_FILTER_B, MH_FILTER_C));
>> +        test.run(List.of("A", "B", "C"));
>> +    }
>> +
>> +    @Test
>> +    public static void testFilterA_B() throws Throwable {
>> +        FilterTest test = new FilterTest(
>> +            filterArguments(MH_TEST, 0, MH_FILTER_A, MH_FILTER_B));
>> +        test.run(List.of("A", "B"));
>> +    }
>> +
>> +    @Test
>> +    public static void testFilterB_C() throws Throwable {
>> +        FilterTest test = new FilterTest(
>> +            filterArguments(MH_TEST, 1, MH_FILTER_B, MH_FILTER_C));
>> +        test.run(List.of("B", "C"));
>> +    }
>> +
>> +    @Test
>> +    public static void testFilterB() throws Throwable {
>> +        FilterTest test = new FilterTest(filterArguments(MH_TEST, 1, MH_FILTER_B));
>> +        test.run(List.of("B"));
>> +    }
>> +
>> +    @Test
>> +    public static void testFilterC() throws Throwable {
>> +        FilterTest test = new FilterTest(filterArguments(MH_TEST, 2, MH_FILTER_C));
>> +        test.run(List.of("C"));
>> +    }
>> +
>> +    static class FilterTest {
>> +        static List<String> filters = new ArrayList<>();
>> +
>> +        final MethodHandle mh;
>> +        FilterTest(MethodHandle mh) {
>> +            this.mh = mh;
>> +        }
>> +
>> +        void run(List<String> expected) throws Throwable {
>> +            filters.clear();
>> +            assertEquals("x-0-z", (String)mh.invokeExact("x", 0, 'z'));
>> +            assertEquals(expected, filters);
>> +        }
>> +
>> +        static String filterA(String s) {
>> +            filters.add("A");
>> +            return s;
>> +        }
>> +
>> +        static int filterB(int value) {
>> +            filters.add("B");
>> +            return value;
>> +        }
>> +
>> +        static char filterC(char c) {
>> +            filters.add("C");
>> +            return c;
>> +        }
>> +
>> +        static String test(String s, int i, char c) {
>> +            return s + "-" + i + "-" + c;
>> +        }
>> +    }
>> +
>> +    static final MethodHandle MH_TEST;
>> +    static final MethodHandle MH_FILTER_A;
>> +    static final MethodHandle MH_FILTER_B;
>> +    static final MethodHandle MH_FILTER_C;
>> +    static final Lookup LOOKUP = MethodHandles.lookup();
>> +
>> +    static {
>> +        try {
>> +            MH_TEST = LOOKUP.findStatic(FilterTest.class, "test",
>> +                methodType(String.class, String.class, int.class, char.class));
>> +            MH_FILTER_A = LOOKUP.findStatic(FilterTest.class, "filterA",
>> +                methodType(String.class, String.class));
>> +            MH_FILTER_B = LOOKUP.findStatic(FilterTest.class, "filterB",
>> +                methodType(int.class, int.class));
>> +            MH_FILTER_C = LOOKUP.findStatic(FilterTest.class, "filterC",
>> +                methodType(char.class, char.class));
>> +        } catch (Exception e) {
>> +            throw new RuntimeException(e);
>> +        }
>> +    }
>> +}
>> 
>> 
>> Thanks,
>> -Aleksey
>> 
>