From 79a62e727a6328bba05b070d481aed9554f1da1b Mon Sep 17 00:00:00 2001 From: Simon Urli Date: Wed, 10 Jan 2018 15:26:42 +0100 Subject: [PATCH 1/3] Replicate bug from issue #1783 --- .../java/spoon/test/method/MethodTest.java | 14 ++ .../spring/PropertyComparator.java | 156 ++++++++++++++++++ 2 files changed, 170 insertions(+) create mode 100644 src/test/resources/noclasspath/spring/PropertyComparator.java diff --git a/src/test/java/spoon/test/method/MethodTest.java b/src/test/java/spoon/test/method/MethodTest.java index 57272fc2d96..f3135cae1c8 100644 --- a/src/test/java/spoon/test/method/MethodTest.java +++ b/src/test/java/spoon/test/method/MethodTest.java @@ -24,6 +24,7 @@ import spoon.reflect.declaration.CtType; import spoon.reflect.declaration.ModifierKind; import spoon.reflect.factory.Factory; +import spoon.reflect.visitor.filter.NamedElementFilter; import spoon.test.delete.testclasses.Adobada; import spoon.test.method.testclasses.Tacos; @@ -85,4 +86,17 @@ public void testGetAllMethods() throws Exception { assertEquals(1, methods.stream().filter(method -> "foo".equals(method.getSimpleName())).count()); } + @Test + public void testGetAllMethodsAdaptingType() throws Exception { + // contract: ... + Launcher l = new Launcher(); + l.getEnvironment().setNoClasspath(true); + l.addInputResource("src/test/resources/noclasspath/spring/PropertyComparator.java"); + l.buildModel(); + + CtType propertyComparator = l.getModel().getElements(new NamedElementFilter(CtType.class, "PropertyComparator")).get(0); + propertyComparator.getAllMethods(); + + } + } diff --git a/src/test/resources/noclasspath/spring/PropertyComparator.java b/src/test/resources/noclasspath/spring/PropertyComparator.java new file mode 100644 index 00000000000..55ac70efdf5 --- /dev/null +++ b/src/test/resources/noclasspath/spring/PropertyComparator.java @@ -0,0 +1,156 @@ +/* + * Copyright 2002-2017 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.beans.support; + +import java.util.Arrays; +import java.util.Collections; +import java.util.Comparator; +import java.util.List; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + +import org.springframework.beans.BeanWrapperImpl; +import org.springframework.beans.BeansException; +import org.springframework.lang.Nullable; +import org.springframework.util.StringUtils; + +/** + * PropertyComparator performs a comparison of two beans, + * evaluating the specified bean property via a BeanWrapper. + * + * @author Juergen Hoeller + * @author Jean-Pierre Pawlak + * @since 19.05.2003 + * @see org.springframework.beans.BeanWrapper + */ +public class PropertyComparator implements Comparator { + + protected final Log logger = LogFactory.getLog(getClass()); + + private final SortDefinition sortDefinition; + + private final BeanWrapperImpl beanWrapper = new BeanWrapperImpl(false); + + + /** + * Create a new PropertyComparator for the given SortDefinition. + * @see MutableSortDefinition + */ + public PropertyComparator(SortDefinition sortDefinition) { + this.sortDefinition = sortDefinition; + } + + /** + * Create a PropertyComparator for the given settings. + * @param property the property to compare + * @param ignoreCase whether upper and lower case in String values should be ignored + * @param ascending whether to sort ascending (true) or descending (false) + */ + public PropertyComparator(String property, boolean ignoreCase, boolean ascending) { + this.sortDefinition = new MutableSortDefinition(property, ignoreCase, ascending); + } + + /** + * Return the SortDefinition that this comparator uses. + */ + public final SortDefinition getSortDefinition() { + return this.sortDefinition; + } + + + @Override + @SuppressWarnings("unchecked") + public int compare(T o1, T o2) { + Object v1 = getPropertyValue(o1); + Object v2 = getPropertyValue(o2); + if (this.sortDefinition.isIgnoreCase() && (v1 instanceof String) && (v2 instanceof String)) { + v1 = ((String) v1).toLowerCase(); + v2 = ((String) v2).toLowerCase(); + } + + int result; + + // Put an object with null property at the end of the sort result. + try { + if (v1 != null) { + result = (v2 != null ? ((Comparable) v1).compareTo(v2) : -1); + } + else { + result = (v2 != null ? 1 : 0); + } + } + catch (RuntimeException ex) { + if (logger.isWarnEnabled()) { + logger.warn("Could not sort objects [" + o1 + "] and [" + o2 + "]", ex); + } + return 0; + } + + return (this.sortDefinition.isAscending() ? result : -result); + } + + /** + * Get the SortDefinition's property value for the given object. + * @param obj the object to get the property value for + * @return the property value + */ + @Nullable + private Object getPropertyValue(Object obj) { + // If a nested property cannot be read, simply return null + // (similar to JSTL EL). If the property doesn't exist in the + // first place, let the exception through. + try { + this.beanWrapper.setWrappedInstance(obj); + return this.beanWrapper.getPropertyValue(this.sortDefinition.getProperty()); + } + catch (BeansException ex) { + logger.info("PropertyComparator could not access property - treating as null for sorting", ex); + return null; + } + } + + + /** + * Sort the given List according to the given sort definition. + *

Note: Contained objects have to provide the given property + * in the form of a bean property, i.e. a getXXX method. + * @param source the input List + * @param sortDefinition the parameters to sort by + * @throws java.lang.IllegalArgumentException in case of a missing propertyName + */ + public static void sort(List source, SortDefinition sortDefinition) throws BeansException { + if (StringUtils.hasText(sortDefinition.getProperty())) { + Collections.sort(source, new PropertyComparator<>(sortDefinition)); + } + } + + /** + * Sort the given source according to the given sort definition. + *

Note: Contained objects have to provide the given property + * in the form of a bean property, i.e. a getXXX method. + * @param source input source + * @param sortDefinition the parameters to sort by + * @throws java.lang.IllegalArgumentException in case of a missing propertyName + */ + public static void sort(Object[] source, SortDefinition sortDefinition) throws BeansException { + if (StringUtils.hasText(sortDefinition.getProperty())) { + Arrays.sort(source, new PropertyComparator<>(sortDefinition)); + } + } + +} From d7c921193d137e710e0675eed54a4658854a304a Mon Sep 17 00:00:00 2001 From: Simon Urli Date: Wed, 10 Jan 2018 16:36:04 +0100 Subject: [PATCH 2/3] Propose a first patch --- .../visitor/AbstractTypingContext.java | 8 +++++-- .../support/visitor/MethodTypingContext.java | 22 +++++++++++++++++++ .../java/spoon/test/method/MethodTest.java | 9 ++++---- .../MethodOverriddingTest.java | 2 +- 4 files changed, 34 insertions(+), 7 deletions(-) diff --git a/src/main/java/spoon/support/visitor/AbstractTypingContext.java b/src/main/java/spoon/support/visitor/AbstractTypingContext.java index c77926e3afb..7701d30973f 100644 --- a/src/main/java/spoon/support/visitor/AbstractTypingContext.java +++ b/src/main/java/spoon/support/visitor/AbstractTypingContext.java @@ -60,7 +60,11 @@ public CtTypeReference adaptType(CtTypeInformation type) { result.setParent(parent); List> actTypeArgs = new ArrayList<>(result.getActualTypeArguments()); for (int i = 0; i < actTypeArgs.size(); i++) { - actTypeArgs.set(i, adaptType(actTypeArgs.get(i)).clone()); + CtTypeReference adaptedTypeArgs = adaptType(actTypeArgs.get(i)); + // for some type argument we might return null to avoid recursive calls + if (adaptedTypeArgs != null) { + actTypeArgs.set(i, adaptedTypeArgs.clone()); + } } result.setActualTypeArguments(actTypeArgs); } @@ -79,7 +83,7 @@ private CtTypeReference adaptTypeParameterReference(CtTypeParameterReference private CtTypeReference adaptTypeParameterReferenceBoundingType(CtTypeParameterReference typeParamRef, CtTypeReference boundingType) { CtTypeParameterReference typeParamRefAdapted = typeParamRef.clone(); typeParamRefAdapted.setParent(typeParamRef.getParent()); - typeParamRefAdapted.setBoundingType(boundingType.equals(boundingType.getFactory().Type().getDefaultBoundingType()) ? null : adaptType(boundingType)); + typeParamRefAdapted.setBoundingType(boundingType.equals(boundingType.getFactory().Type().getDefaultBoundingType()) ? boundingType.getFactory().Type().getDefaultBoundingType() : adaptType(boundingType)); return typeParamRefAdapted; } diff --git a/src/main/java/spoon/support/visitor/MethodTypingContext.java b/src/main/java/spoon/support/visitor/MethodTypingContext.java index ee62c083ef8..fd4b94ed542 100644 --- a/src/main/java/spoon/support/visitor/MethodTypingContext.java +++ b/src/main/java/spoon/support/visitor/MethodTypingContext.java @@ -35,6 +35,7 @@ import spoon.reflect.reference.CtExecutableReference; import spoon.reflect.reference.CtTypeParameterReference; import spoon.reflect.reference.CtTypeReference; +import spoon.reflect.reference.CtWildcardReference; /** * For the scope method or constructor and super type hierarchy of it's declaring type, @@ -249,6 +250,27 @@ private boolean isSameMethodFormalTypeParameter(CtTypeParameter scopeParam, CtTy return idxOfScopeBoundTypeParam == idxOfSuperBoundTypeParam; } } + if (scopeBound.getActualTypeArguments().size() != superBound.getActualTypeArguments().size()) { + return false; + } + + for (int i = 0; i < scopeBound.getActualTypeArguments().size(); i++) { + CtTypeReference scopeBoundATArg = scopeBound.getActualTypeArguments().get(i); + CtTypeReference superBoundATArg = superBound.getActualTypeArguments().get(i); + + if (scopeBoundATArg instanceof CtWildcardReference && superBoundATArg instanceof CtWildcardReference) { + CtWildcardReference scopeBoundWildcard = (CtWildcardReference) scopeBoundATArg; + + // we are in a case where we compare Thing with Thing + // here we choose to not care about X to avoid recursive calls, for example in the case of: + // T extends Comparable + if (scopeBoundWildcard.getBoundingType().equals(scopeMethod.getFactory().Type().getDefaultBoundingType())) { + return true; + } + } + } + + CtTypeReference superBoundAdapted = adaptType(superBound); if (superBoundAdapted == null) { return false; diff --git a/src/test/java/spoon/test/method/MethodTest.java b/src/test/java/spoon/test/method/MethodTest.java index f3135cae1c8..bcb4a504798 100644 --- a/src/test/java/spoon/test/method/MethodTest.java +++ b/src/test/java/spoon/test/method/MethodTest.java @@ -88,15 +88,16 @@ public void testGetAllMethods() throws Exception { @Test public void testGetAllMethodsAdaptingType() throws Exception { - // contract: ... + // contract: AbstractTypingContext should not enter in recursive calls when resolving autoreferenced bounding type + // such as T extends Comparable Launcher l = new Launcher(); l.getEnvironment().setNoClasspath(true); l.addInputResource("src/test/resources/noclasspath/spring/PropertyComparator.java"); l.buildModel(); - CtType propertyComparator = l.getModel().getElements(new NamedElementFilter(CtType.class, "PropertyComparator")).get(0); - propertyComparator.getAllMethods(); - + CtType propertyComparator = l.getModel().getElements(new NamedElementFilter(CtType.class, "PropertyComparator")).get(0); + CtMethod method = propertyComparator.getMethodsByName("compare").get(0); + assertEquals("compare(T,T)", method.getSignature()); } } diff --git a/src/test/java/spoon/test/method_overriding/MethodOverriddingTest.java b/src/test/java/spoon/test/method_overriding/MethodOverriddingTest.java index d8ed47ef2f8..b47fd2a56cc 100644 --- a/src/test/java/spoon/test/method_overriding/MethodOverriddingTest.java +++ b/src/test/java/spoon/test/method_overriding/MethodOverriddingTest.java @@ -46,7 +46,7 @@ private void checkMethodOverride(BiFunction, CtMethod, Boolean> i private void combine(List value, int start, BiFunction, CtMethod, Boolean> isOverriding) { CtMethod m1 = value.get(start); - if(start+1 Date: Fri, 12 Jan 2018 10:18:10 +0100 Subject: [PATCH 3/3] Simplify test resource and fix test --- .../java/spoon/test/method/MethodTest.java | 14 ++- .../spring/PropertyComparator.java | 110 +----------------- 2 files changed, 13 insertions(+), 111 deletions(-) diff --git a/src/test/java/spoon/test/method/MethodTest.java b/src/test/java/spoon/test/method/MethodTest.java index bcb4a504798..0b585aa0af0 100644 --- a/src/test/java/spoon/test/method/MethodTest.java +++ b/src/test/java/spoon/test/method/MethodTest.java @@ -34,6 +34,7 @@ import java.util.Set; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static spoon.testing.utils.ModelUtils.build; import static spoon.testing.utils.ModelUtils.buildClass; @@ -96,8 +97,17 @@ public void testGetAllMethodsAdaptingType() throws Exception { l.buildModel(); CtType propertyComparator = l.getModel().getElements(new NamedElementFilter(CtType.class, "PropertyComparator")).get(0); - CtMethod method = propertyComparator.getMethodsByName("compare").get(0); - assertEquals("compare(T,T)", method.getSignature()); + Set> allMethods = propertyComparator.getAllMethods(); + + boolean compareFound = false; + for (CtMethod method : allMethods) { + if (method.getSimpleName().equals("compare")) { + assertEquals("compare(T,T)", method.getSignature()); + compareFound = true; + } + } + + assertTrue(compareFound); } } diff --git a/src/test/resources/noclasspath/spring/PropertyComparator.java b/src/test/resources/noclasspath/spring/PropertyComparator.java index 55ac70efdf5..449d4c43bf4 100644 --- a/src/test/resources/noclasspath/spring/PropertyComparator.java +++ b/src/test/resources/noclasspath/spring/PropertyComparator.java @@ -40,117 +40,9 @@ */ public class PropertyComparator implements Comparator { - protected final Log logger = LogFactory.getLog(getClass()); - - private final SortDefinition sortDefinition; - - private final BeanWrapperImpl beanWrapper = new BeanWrapperImpl(false); - - - /** - * Create a new PropertyComparator for the given SortDefinition. - * @see MutableSortDefinition - */ - public PropertyComparator(SortDefinition sortDefinition) { - this.sortDefinition = sortDefinition; - } - - /** - * Create a PropertyComparator for the given settings. - * @param property the property to compare - * @param ignoreCase whether upper and lower case in String values should be ignored - * @param ascending whether to sort ascending (true) or descending (false) - */ - public PropertyComparator(String property, boolean ignoreCase, boolean ascending) { - this.sortDefinition = new MutableSortDefinition(property, ignoreCase, ascending); - } - - /** - * Return the SortDefinition that this comparator uses. - */ - public final SortDefinition getSortDefinition() { - return this.sortDefinition; - } - - @Override @SuppressWarnings("unchecked") public int compare(T o1, T o2) { - Object v1 = getPropertyValue(o1); - Object v2 = getPropertyValue(o2); - if (this.sortDefinition.isIgnoreCase() && (v1 instanceof String) && (v2 instanceof String)) { - v1 = ((String) v1).toLowerCase(); - v2 = ((String) v2).toLowerCase(); - } - - int result; - - // Put an object with null property at the end of the sort result. - try { - if (v1 != null) { - result = (v2 != null ? ((Comparable) v1).compareTo(v2) : -1); - } - else { - result = (v2 != null ? 1 : 0); - } - } - catch (RuntimeException ex) { - if (logger.isWarnEnabled()) { - logger.warn("Could not sort objects [" + o1 + "] and [" + o2 + "]", ex); - } - return 0; - } - - return (this.sortDefinition.isAscending() ? result : -result); + return o1.toString().compareTo(o2.toString()); } - - /** - * Get the SortDefinition's property value for the given object. - * @param obj the object to get the property value for - * @return the property value - */ - @Nullable - private Object getPropertyValue(Object obj) { - // If a nested property cannot be read, simply return null - // (similar to JSTL EL). If the property doesn't exist in the - // first place, let the exception through. - try { - this.beanWrapper.setWrappedInstance(obj); - return this.beanWrapper.getPropertyValue(this.sortDefinition.getProperty()); - } - catch (BeansException ex) { - logger.info("PropertyComparator could not access property - treating as null for sorting", ex); - return null; - } - } - - - /** - * Sort the given List according to the given sort definition. - *

Note: Contained objects have to provide the given property - * in the form of a bean property, i.e. a getXXX method. - * @param source the input List - * @param sortDefinition the parameters to sort by - * @throws java.lang.IllegalArgumentException in case of a missing propertyName - */ - public static void sort(List source, SortDefinition sortDefinition) throws BeansException { - if (StringUtils.hasText(sortDefinition.getProperty())) { - Collections.sort(source, new PropertyComparator<>(sortDefinition)); - } - } - - /** - * Sort the given source according to the given sort definition. - *

Note: Contained objects have to provide the given property - * in the form of a bean property, i.e. a getXXX method. - * @param source input source - * @param sortDefinition the parameters to sort by - * @throws java.lang.IllegalArgumentException in case of a missing propertyName - */ - public static void sort(Object[] source, SortDefinition sortDefinition) throws BeansException { - if (StringUtils.hasText(sortDefinition.getProperty())) { - Arrays.sort(source, new PropertyComparator<>(sortDefinition)); - } - } - }