Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow the usage of external methods with MapProperty.Use, MapPropertyFromSource.Use and MapValue.Use #1298

Open
Kibonik opened this issue May 24, 2024 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@Kibonik
Copy link

Kibonik commented May 24, 2024

Describe the bug

UseStaticMapper doesnt work with MapPropertyFromSource

Code

	public class SomeTest
	{
		public int Id { get; set; }
		public string? Number { get; set; }
		public string? Prefix { get; set; }
	}
	public class SomeTestDto
	{
		public int Id { get; set; }
		public string? NumberFull { get; set; }


	}
	[Mapper]
	[UseStaticMapper(typeof(TestMapper))]
	public static partial class SomeMapper
	{
		[MapPropertyFromSource(nameof(SomeTestDto.NumberFull), Use = nameof(MapProducer))] //or Use = nameof(TestMapper.MapProducer)
		public static partial SomeTestDto MapMe(SomeTest s);
	}

	public static class TestMapper
	{
		public static string? MapProducer(SomeTest s)
				   => s.Prefix + s.Number;
	}

Generated code

            public static partial global::TimberErp.Shared.Models.LoadMovesDtoMapper.SomeTestDto MapMe(global::TimberErp.Shared.Models.LoadMovesDtoMapper.SomeTest s)
            {
                var target = new global::TimberErp.Shared.Models.LoadMovesDtoMapper.SomeTestDto();
                target.Id = s.Id;
                target.NumberFull = s.ToString();
                return target;
            }

Error

Severity	Code	Description	Project	File	Line	Suppression State
Error	CS0103	The name 'MapProducer' does not exist in the current context	
Error	RMG061	The referenced mapping named MapProducer was not found	
@Kibonik Kibonik added the bug Something isn't working label May 24, 2024
@latonz latonz added enhancement New feature or request and removed bug Something isn't working labels May 28, 2024
@latonz latonz changed the title UseStaticMapper doesnt work with MapPropertyFromSource Allow the usage of external methods with MapProperty.Use and MapPropertyFromSource.Use May 28, 2024
@latonz
Copy link
Contributor

latonz commented May 28, 2024

This is not supported and isn't expected to work. Right now Use does only allow methods of the same class and its type hierarchy. This needs some concept on how to address these external methods (probably use a "full-name" concept similar to the one we use for properties (see docs)?)...

@latonz latonz changed the title Allow the usage of external methods with MapProperty.Use and MapPropertyFromSource.Use Allow the usage of external methods with MapProperty.Use, MapPropertyFromSource.Use and MapValue.Use Jul 22, 2024
@IanKemp
Copy link
Contributor

IanKemp commented Oct 4, 2024

100% support the @nameof / "fullnameof" special-casing as a way to achieve this feature, especially since the C# language implementers can't be bothered to implement it. I would consider this a "small feature" as outlined in your contribution docs, @latonz would you agree and is this something you would be happy to entertain a PR to implement?

@latonz
Copy link
Contributor

latonz commented Oct 4, 2024

@IanKemp Go for it! 😊 I'm looking forward to review your PR.
Note: the implementation should work for all possible variations (e.g. a static method on a static type, an instance method on a field of the mapper class, ...).

@IanKemp
Copy link
Contributor

IanKemp commented Oct 5, 2024

@latonz Would appreciate a pointer on the architectural approach before I go ahead and jump into implementation.

From my initial investigation it seems that the simplest way to implement this would be to change MemberMappingConfiguration.Use from being typed as string? to StringMemberPath? and then special-case handling of the latter type in AttributeDataAccessor.BuildArgumentValue (which currently is only handling the non-nullable version). I'd probably also have to pass the targetType to CreateMemberPath such that the latter can differentiate between "short full nameof" (current behaviour, used for MemberMappingConfiguration.Source and Target) and "actual full nameof" (new behaviour for Use). This approach is not great because if there should ever be a desire to change the nullability of any of the three aforementioned properties, then their behaviour will also implicitly change.

Then I looked at making StringMemberPath an abstract record class as opposed to the record struct it currently is; having two derived types, ShortFullNameOfStringMemberPath for Source and Target and FullNameOfStringMemberPath for Use; and then special-casing BuildArgumentValue and CreateMemberPath on these discrete derived types. The problem there is that changing struct to class has semantics around allocation and therefore performance, and of course source generators (and indeed anything Roslyn-related) is performance-sensitive, so this seems like a bit of an invasive change for the desired feature.

Thoughts?

@latonz
Copy link
Contributor

latonz commented Oct 6, 2024

@IanKemp instead of adding nullable value type handling to all of the arms in AttributeDataAccessor.BuildArgumentValue, we could simply use Nullable.GetUnderlyingType and overwrite the targetType if the underlying type is non-null.

The second problem with the .Skip(1): i really dislike this Skip(1) and I planned already to rework this to get the fullnameof feature to work also with namespaced/nested type references. I think with the help of SemanticModel.GetOperation we could directly get the full path to the target member. I think if we could use such an approach, there would be no need for a string representation of the member path, instead a symbol member path could be used. If that doesn't work out, I'd probably simply create a completely separate FullStringMemberPath for now (and document the differences in a xml comment).

@latonz
Copy link
Contributor

latonz commented Oct 6, 2024

@IanKemp I pushed my experiments here #1518, see especially Riok.Mapperly.Configuration.SymbolMemberPath which could be what you need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants