#394 Fix for making ForgedMethod names unique when the param and returntype have similar simple names

processor/src/main/java/org/mapstruct/ap/model/MapMappingMethod.java

@@ -27,7 +27,6 @@ import org.mapstruct.ap.model.assignment.Assignment;
 import org.mapstruct.ap.model.assignment.LocalVarWrapper;
 import org.mapstruct.ap.model.common.Parameter;
 import org.mapstruct.ap.model.common.Type;
-import org.mapstruct.ap.model.common.TypeFactory;
 import org.mapstruct.ap.model.source.Method;
 import org.mapstruct.ap.prism.NullValueMappingPrism;
 import org.mapstruct.ap.util.MapperConfig;
@@ -46,7 +45,6 @@ public class MapMappingMethod extends MappingMethod {
     private final MethodReference factoryMethod;
     private final boolean overridden;
     private final boolean mapNullToDefault;
-    private final TypeFactory typeFactory;
 
     public static class Builder {
 
@@ -153,14 +151,13 @@ public class MapMappingMethod extends MappingMethod {
                 keyAssignment,
                 valueAssignment,
                 factoryMethod,
-                mapNullToDefault,
+                mapNullToDefault
-                ctx.getTypeFactory()
             );
         }
     }
 
     private MapMappingMethod(Method method, Assignment keyAssignment, Assignment valueAssignment,
-                             MethodReference factoryMethod, boolean mapNullToDefault, TypeFactory typeFactory) {
+                             MethodReference factoryMethod, boolean mapNullToDefault) {
         super( method );
 
         this.keyAssignment = keyAssignment;
@@ -168,7 +165,6 @@ public class MapMappingMethod extends MappingMethod {
         this.factoryMethod = factoryMethod;
         this.overridden = method.overridesMethod();
         this.mapNullToDefault = mapNullToDefault;
-        this.typeFactory = typeFactory;
     }
 
     public Parameter getSourceParameter() {

processor/src/main/java/org/mapstruct/ap/model/MappingBuilderContext.java

@@ -168,6 +168,25 @@ public class MappingBuilderContext {
         return mappingsToGenerate;
     }
 
+    public List<String> getNamesOfMappingsToGenerate() {
+        List<String> nameList = new ArrayList<String>();
+        for ( MappingMethod method : mappingsToGenerate ) {
+            nameList.add( method.getName() );
+        }
+        return nameList;
+    }
+
+    public MappingMethod getExistingMappingMethod(MappingMethod newMappingMethod) {
+        MappingMethod existingMappingMethod = null;
+        for ( MappingMethod mappingMethod : mappingsToGenerate ) {
+            if ( newMappingMethod.equals( mappingMethod ) ) {
+                existingMappingMethod = mappingMethod;
+                break;
+            }
+        }
+        return existingMappingMethod;
+    }
+
     public Set<VirtualMappingMethod> getUsedVirtualMappings() {
         return mappingResolver.getUsedVirtualMappings();
     }

processor/src/main/java/org/mapstruct/ap/model/PropertyMapping.java

@@ -48,10 +48,9 @@ import static org.mapstruct.ap.model.assignment.Assignment.AssignmentType.TYPE_C
 import static org.mapstruct.ap.model.assignment.Assignment.AssignmentType.TYPE_CONVERTED_MAPPED;
 
 /**
- * Represents the mapping between a source and target property, e.g. from
+ * Represents the mapping between a source and target property, e.g. from {@code String Source#foo} to
- * {@code String Source#foo} to {@code int Target#bar}. Name and type of source
+ * {@code int Target#bar}. Name and type of source and target property can differ. If they have different types, the
- * and target property can differ. If they have different types, the mapping
+ * mapping must either refer to a mapping method or a conversion.
- * must either refer to a mapping method or a conversion.
  *
  * @author Gunnar Morling
  */
@@ -268,7 +267,6 @@ public class PropertyMapping extends ModelElement {
             return result;
         }
 
-
         private Type getSourceType() {
 
             Parameter sourceParam = sourceReference.getParameter();
@@ -304,16 +302,17 @@ public class PropertyMapping extends ModelElement {
                 PropertyEntry lastPropertyEntry = propertyEntries.get( propertyEntries.size() - 1 );
 
                 // forge a method from the parameter type to the last entry type.
-                String forgedMethodName = Strings.joinAndCamelize( sourceReference.getElementNames() );
+                String forgedName = Strings.joinAndCamelize( sourceReference.getElementNames() );
-                ForgedMethod methodToGenerate = new ForgedMethod(
+                forgedName = Strings.getSaveVariableName( forgedName, ctx.getNamesOfMappingsToGenerate() );
-                    forgedMethodName,
+                ForgedMethod methodRef = new ForgedMethod(
+                    forgedName,
                     sourceReference.getParameter().getType(),
                     lastPropertyEntry.getType(),
                     method.getExecutable()
                 );
                 NestedPropertyMappingMethod.Builder builder = new NestedPropertyMappingMethod.Builder();
                 NestedPropertyMappingMethod nestedPropertyMapping = builder
-                    .method( methodToGenerate )
+                    .method( methodRef )
                     .propertyEntries( sourceReference.getPropertyEntries() )
                     .build();
 
@@ -321,8 +320,11 @@ public class PropertyMapping extends ModelElement {
                 if ( !ctx.getMappingsToGenerate().contains( nestedPropertyMapping ) ) {
                     ctx.getMappingsToGenerate().add( nestedPropertyMapping );
                 }
+                else {
+                    forgedName = ctx.getExistingMappingMethod( nestedPropertyMapping ).getName();
+                }
 
-                return forgedMethodName + "( " + sourceParam.getName() + " )";
+                return forgedName + "( " + sourceParam.getName() + " )";
             }
         }
 
@@ -374,43 +376,70 @@ public class PropertyMapping extends ModelElement {
                                                      ExecutableElement element) {
 
             Assignment assignment = null;
+
+            String name = getName( sourceType, targetType );
+            name = Strings.getSaveVariableName( name, ctx.getNamesOfMappingsToGenerate() );
+
             if ( (sourceType.isCollectionType() || sourceType.isArrayType())
                 && (targetType.isCollectionType() || targetType.isArrayType()) ) {
 
-                ForgedMethod methodToGenerate = new ForgedMethod( sourceType, targetType, element );
+                ForgedMethod methodRef = new ForgedMethod( name, sourceType, targetType, element );
                 IterableMappingMethod.Builder builder = new IterableMappingMethod.Builder();
 
-
                 IterableMappingMethod iterableMappingMethod = builder
                     .mappingContext( ctx )
-                    .method( methodToGenerate )
+                    .method( methodRef )
                     .build();
 
                 if ( !ctx.getMappingsToGenerate().contains( iterableMappingMethod ) ) {
                     ctx.getMappingsToGenerate().add( iterableMappingMethod );
                 }
-                assignment = AssignmentFactory.createMethodReference( methodToGenerate, null, targetType );
+                else {
+                    String existingName = ctx.getExistingMappingMethod( iterableMappingMethod ).getName();
+                    methodRef = new ForgedMethod( existingName, methodRef );
+                }
+
+                assignment = AssignmentFactory.createMethodReference( methodRef, null, targetType );
                 assignment.setAssignment( AssignmentFactory.createDirect( sourceReference ) );
 
             }
             else if ( sourceType.isMapType() && targetType.isMapType() ) {
 
-                ForgedMethod methodToGenerate = new ForgedMethod( sourceType, targetType, element );
+                ForgedMethod methodRef = new ForgedMethod( name, sourceType, targetType, element );
 
                 MapMappingMethod.Builder builder = new MapMappingMethod.Builder();
                 MapMappingMethod mapMappingMethod = builder
                     .mappingContext( ctx )
-                    .method( methodToGenerate )
+                    .method( methodRef )
                     .build();
 
                 if ( !ctx.getMappingsToGenerate().contains( mapMappingMethod ) ) {
                     ctx.getMappingsToGenerate().add( mapMappingMethod );
                 }
-                assignment = AssignmentFactory.createMethodReference( methodToGenerate, null, targetType );
+                else {
+                    String existingName = ctx.getExistingMappingMethod( mapMappingMethod ).getName();
+                    methodRef = new ForgedMethod( existingName, methodRef );
+                }
+                assignment = AssignmentFactory.createMethodReference( methodRef, null, targetType );
                 assignment.setAssignment( AssignmentFactory.createDirect( sourceReference ) );
             }
             return assignment;
         }
+
+        private String getName(Type sourceType, Type targetType) {
+            String fromName = getName( sourceType );
+            String toName = getName( targetType );
+            return Strings.decapitalize( fromName + "To" + toName );
+        }
+
+        private String getName(Type type) {
+            StringBuilder builder = new StringBuilder();
+            for ( Type typeParam : type.getTypeParameters() ) {
+                builder.append( typeParam.getName().replace( "[]", "Array" ) );
+            }
+            builder.append( type.getName().replace( "[]", "Array" ) );
+            return builder.toString();
+        }
     }
 
     public static class ConstantMappingBuilder {
@@ -554,10 +583,8 @@ public class PropertyMapping extends ModelElement {
             return new PropertyMapping( targetAccessor.getSimpleName().toString(), targetType, assignment );
         }
 
-
     }
 
-
     // Constructor for creating mappings of constant expressions.
     private PropertyMapping(String targetAccessorName, Type targetType, Assignment propertyAssignment) {
         this( null, targetAccessorName, targetType, propertyAssignment );
@@ -596,10 +623,10 @@ public class PropertyMapping extends ModelElement {
 
     @Override
     public String toString() {
-        return "PropertyMapping {" +
+        return "PropertyMapping {"
-            "\n    targetName='" + targetAccessorName + "\'," +
+            + "\n    targetName='" + targetAccessorName + "\',"
-            "\n    targetType=" + targetType + "," +
+            + "\n    targetType=" + targetType + ","
-            "\n    propertyAssignment=" + assignment +
+            + "\n    propertyAssignment=" + assignment
-            "\n}";
+            + "\n}";
     }
 }

processor/src/main/java/org/mapstruct/ap/model/source/ForgedMethod.java

@@ -41,28 +41,6 @@ public class ForgedMethod implements Method {
     private final String name;
     private final ExecutableElement positionHintElement;
 
-    /**
-     * Creates a new forged method.
-     * <p>
-     * The name will be based on the source type name and target type name.
-     *
-     * @param sourceType the source type
-     * @param targetType the target type.
-     * @param positionHintElement element used to for reference to the position in the source file.
-     */
-    public ForgedMethod(Type sourceType, Type targetType, ExecutableElement positionHintElement) {
-        String sourceParamName = Strings.decapitalize( sourceType.getName().replace( "[]", "" ) );
-        String sourceParamSafeName = Strings.getSaveVariableName( sourceParamName );
-        this.parameters = Arrays.asList( new Parameter( sourceParamSafeName, sourceType ) );
-        this.returnType = targetType;
-
-        String fromName = getName( parameters.iterator().next().getType() );
-        String toName = getName( returnType );
-        name = Strings.decapitalize( fromName + "To" + toName );
-
-        this.positionHintElement = positionHintElement;
-    }
-
      /**
      * Creates a new forged method with the given name.
      *
@@ -80,14 +58,16 @@ public class ForgedMethod implements Method {
         this.positionHintElement = positionHintElement;
     }
 
-    private String getName(Type type) {
+    /**
-        StringBuilder builder = new StringBuilder();
+     * creates a new ForgedMethod with the same arguments but with a new name
-        for ( Type typeParam : type.getTypeParameters() ) {
+     * @param name the new name
-
+     * @param forgedMethod existing forge method
-            builder.append( typeParam.getName().replace( "[]", "Array" ) );
+     */
-        }
+    public ForgedMethod(String name, ForgedMethod forgedMethod) {
-        builder.append( type.getName().replace( "[]", "Array" ) );
+        this.parameters = forgedMethod.parameters;
-        return builder.toString();
+        this.returnType = forgedMethod.returnType;
+        this.positionHintElement = forgedMethod.positionHintElement;
+        this.name = name;
     }
 
     @Override

processor/src/main/resources/org.mapstruct.ap.model.assignment.LocalVarWrapper.ftl

@@ -19,9 +19,9 @@
 
 -->
 <#if (exceptionTypes?size == 0) >
-    <#if !ext.isTargetDefined?? >${ext.targetType.name}</#if> ${ext.targetAccessorName} = <@includeModel object=assignment targetType=ext.targetType raw=ext.raw/>;
+    <#if !ext.isTargetDefined?? ><@includeModel object=ext.targetType/></#if> ${ext.targetAccessorName} = <@includeModel object=assignment targetType=ext.targetType raw=ext.raw/>;
 <#else>
-    <#if !ext.isTargetDefined?? >${ext.targetType.name} ${ext.targetAccessorName};</#if>
+    <#if !ext.isTargetDefined?? ><@includeModel object=ext.targetType/> ${ext.targetAccessorName};</#if>
     try {
         ${ext.targetAccessorName} = <@includeModel object=assignment targetType=ext.targetType raw=ext.raw/>;
     }

processor/src/test/java/org/mapstruct/ap/test/bugs/_394/SameClassNameInDifferentPackageTest.java

@@ -16,12 +16,12 @@
  *  See the License for the specific language governing permissions and
  *  limitations under the License.
  */
-package org.mapstruct.ap.test.collection.map;
+package org.mapstruct.ap.test.bugs._394;
 
 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.mapstruct.ap.test.collection.map.source.AnotherCar;
+import org.mapstruct.ap.test.bugs._394.source.AnotherCar;
-import org.mapstruct.ap.test.collection.map.source.Cars;
+import org.mapstruct.ap.test.bugs._394.source.Cars;
 import org.mapstruct.ap.testutil.IssueKey;
 import org.mapstruct.ap.testutil.WithClasses;
 import org.mapstruct.ap.testutil.runner.AnnotationProcessorTestRunner;
@@ -31,29 +31,29 @@ import java.util.Map;
 
 import static org.fest.assertions.Assertions.assertThat;
 
-@WithClasses({ SameNameForSourceAndTargetCarsMapper.class, Cars.class, org.mapstruct.ap.test.collection.map.targets.Cars
+@WithClasses( {
-        .class, AnotherCar.class, org.mapstruct.ap.test.collection.map.targets.AnotherCar.class })
+    SameNameForSourceAndTargetCarsMapper.class,
+    Cars.class,
+    org.mapstruct.ap.test.bugs._394.target.Cars.class,
+    AnotherCar.class,
+    org.mapstruct.ap.test.bugs._394.target.AnotherCar.class
+} )
 @IssueKey("394")
 @RunWith(AnnotationProcessorTestRunner.class)
 public class SameClassNameInDifferentPackageTest {
+
     @Test
     public void shouldCreateMapMethodImplementation() {
         Map<String, AnotherCar> values = new HashMap<String, AnotherCar>();
         //given
-        AnotherCar honda = new AnotherCar(
+        AnotherCar honda = new AnotherCar( "Honda", 2 );
-                "Honda",
+        AnotherCar toyota = new AnotherCar( "Toyota", 2);
-                2
-        );
-        AnotherCar toyota = new AnotherCar(
-                "Toyota",
-                2
-        );
         values.put( "Honda", honda );
         values.put( "Toyota", toyota );
 
         Cars cars = new Cars();
         cars.setMakeToCar( values );
-        org.mapstruct.ap.test.collection.map.targets.Cars targetCars = SameNameForSourceAndTargetCarsMapper.INSTANCE
+        org.mapstruct.ap.test.bugs._394.target.Cars targetCars = SameNameForSourceAndTargetCarsMapper.INSTANCE
                 .sourceCarsToTargetCars( cars );
         assertThat( targetCars ).isNotNull();
         assertThat( targetCars.getMakeToCar() ).hasSize( 2 );

processor/src/test/java/org/mapstruct/ap/test/bugs/_394/SameNameForSourceAndTargetCarsMapper.java

@@ -16,16 +16,17 @@
  *  See the License for the specific language governing permissions and
  *  limitations under the License.
  */
-package org.mapstruct.ap.test.collection.map;
+package org.mapstruct.ap.test.bugs._394;
 
 import org.mapstruct.Mapper;
 import org.mapstruct.Mapping;
 import org.mapstruct.Mappings;
-import org.mapstruct.ap.test.collection.map.source.Cars;
+import org.mapstruct.ap.test.bugs._394.source.Cars;
-import org.mapstruct.ap.test.collection.map.targets.AnotherCar;
+import org.mapstruct.ap.test.bugs._394.target.AnotherCar;
 import org.mapstruct.factory.Mappers;
 
 import java.util.List;
+import org.mapstruct.InheritInverseConfiguration;
 
 @Mapper
 public interface SameNameForSourceAndTargetCarsMapper {
@@ -35,8 +36,19 @@ public interface SameNameForSourceAndTargetCarsMapper {
     @Mappings({
         @Mapping(source = "numberOfSeats", target = "seatCount")
     })
-    AnotherCar carToCarDto(org.mapstruct.ap.test.collection.map.source.AnotherCar car);
+    AnotherCar sourceCarToTargetCar(org.mapstruct.ap.test.bugs._394.source.AnotherCar car);
+
+    List<AnotherCar> sourceCarListToTargetCarList(List<org.mapstruct.ap.test.bugs._394.source.AnotherCar> cars);
+
+    org.mapstruct.ap.test.bugs._394.target.Cars sourceCarsToTargetCars(Cars source);
+
+    // Reverse mehtods
+
+    @InheritInverseConfiguration
+    org.mapstruct.ap.test.bugs._394.source.AnotherCar targetCarToSourceCar(AnotherCar car);
+
+    List<org.mapstruct.ap.test.bugs._394.source.AnotherCar> targetCarListToSourceCarList(List<AnotherCar> cars);
+
+    Cars targetCarsToSourceCars(org.mapstruct.ap.test.bugs._394.target.Cars source);
 
-    List<AnotherCar> carsToCarDtos(List<org.mapstruct.ap.test.collection.map.source.AnotherCar> cars);
-    org.mapstruct.ap.test.collection.map.targets.Cars sourceCarsToTargetCars(Cars source);
 }

processor/src/test/java/org/mapstruct/ap/test/bugs/_394/source/AnotherCar.java

@@ -16,7 +16,7 @@
  *  See the License for the specific language governing permissions and
  *  limitations under the License.
  */
-package org.mapstruct.ap.test.collection.map.source;
+package org.mapstruct.ap.test.bugs._394.source;
 
 public class AnotherCar {
 

processor/src/test/java/org/mapstruct/ap/test/bugs/_394/source/Cars.java

@@ -16,7 +16,7 @@
  *  See the License for the specific language governing permissions and
  *  limitations under the License.
  */
-package org.mapstruct.ap.test.collection.map.source;
+package org.mapstruct.ap.test.bugs._394.source;
 
 import java.util.Map;
 

processor/src/test/java/org/mapstruct/ap/test/bugs/_394/target/AnotherCar.java

@@ -16,7 +16,7 @@
  *  See the License for the specific language governing permissions and
  *  limitations under the License.
  */
-package org.mapstruct.ap.test.collection.map.targets;
+package org.mapstruct.ap.test.bugs._394.target;
 
 public class AnotherCar {
 

processor/src/test/java/org/mapstruct/ap/test/bugs/_394/target/Cars.java

@@ -16,7 +16,7 @@
  *  See the License for the specific language governing permissions and
  *  limitations under the License.
  */
-package org.mapstruct.ap.test.collection.map.targets;
+package org.mapstruct.ap.test.bugs._394.target;
 
 import java.util.Map;