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

Nullable error when target type comes from external reference #690

Closed
PaulVrugt opened this issue Aug 28, 2023 · 8 comments · Fixed by #715
Closed

Nullable error when target type comes from external reference #690

PaulVrugt opened this issue Aug 28, 2023 · 8 comments · Fixed by #715
Labels
bug Something isn't working

Comments

@PaulVrugt
Copy link

Describe the bug
Nullable error when target type comes from external reference

To Reproduce
Steps to reproduce the behavior:
Download the sample project from #679

Expected behavior
No nullable erros

Code snippets
source:

public class Source
{
    public SubSource[]? Property1 { get; set; }
}

public class SubSource
{
    public Guid Id { get; set; }
}

and a mapper:

[Mapper]
internal static partial class EntityMapper
{
    internal static partial Target ToMutationIssue(Source issueDto);
}

and the target, but this target is included via a references project:

public class Target
{
    public SubTarget[]? Property1 { get; set; }
}

public class SubTarget
{
    public Guid Id { get; set; }
}

Environment (please complete the following information):

  • Mapperly Version: 3.1.0
  • .NET Version: .net 7
  • Target Framework: .net 7
  • IDE: Visual Studio v17.6.6
  • OS: windows 11

Additional context
It only happens when the target class is in a references project

@latonz latonz added the bug Something isn't working label Aug 28, 2023
@latonz
Copy link
Contributor

latonz commented Sep 1, 2023

I can reproduce the issue, but I don't really understand it.
The relevant generated code looks like this:

...
if (issueDto.Property1 != null)
{
     target.Property1 = MapToSubTargetArray(issueDto.Property1);
}
...
private static global::dtoTest.SubTarget[] MapToSubTargetArray(global::NullableMapperlyTest.SubSource[] source)
...

The compiler reports error CS8619: Nullability of reference types in value of type 'SubTarget?[]' doesn't match target type 'SubTarget[]', but the generated method returns global::dtoTest.SubTarget[] and has a #nullable enable directive at the top. If I copy the generated mapper into a cs file into the project and comment the [Mapper] attribute it works just fine. Although the referenced project has nullables enabled and the property on the target is of type SubTarget[]?, and not SubTarget?[] as the compiler reports. I'm not sure if this is really a problem of Mapperly or if it is a problem of Roslyn. Especially since the generated code works 1:1 if copied to the the project for which it is generated.

@TimothyMakkison what are your thoughts on this?

@PaulVrugt
Copy link
Author

Yeah my brain also crashed investigating what was going wrong. But the problem in our case is that ALL dto's are coming from a referenced project, and we treat all warnings as errors in our project, so this issue is preventing our code base to build.

As far as I could find mapperly doesn't have an option to disable nullable context in the generated code does it? Because that would be a workaround for us for the time being

@latonz
Copy link
Contributor

latonz commented Sep 1, 2023

@PaulVrugt nope Mapperly doesn't have such an option. The problem only exiss on arrays, correct?

Sorry that you can't use Mapperly for now 🙄

@TimothyMakkison
Copy link
Collaborator

TimothyMakkison commented Sep 1, 2023

The nullability being on the array element threw me + the difference between VS and rider.

Perhaps in external references, arrays don't correctly annotate array element/generic types, so they all annotated with None. This is then upgraded to Annotated by mapperly.

I'm strugglng to debug this. I can recreate the issue if I look at the generated code. Running it in debug mode generates the following code with no nullability issues 😭

public static partial global::Lib.ContainerDto MapTo(global::Riok.Mapperly.Sample.Container src)
{
    var target = new global::Lib.ContainerDto();
    target.Arrays = MapToElementDtoArray(src.Arrays);
    return target;
}
private static global::Lib.ElementDto MapToElementDto(global::Riok.Mapperly.Sample.Element source)
{
    var target = new global::Lib.ElementDto();
    target.Value = source.Value;
    return target;
}
private static global::Lib.ElementDto[] MapToElementDtoArray(global::Riok.Mapperly.Sample.Element[] source)
{
    var target = new global::Lib.ElementDto[source.Length];
    for (var i = 0; i < source.Length; i++)
    {
        target[i] = MapToElementDto(source[i]);
    }

    return target;
}

@TimothyMakkison
Copy link
Collaborator

I think I've fixed the issue. Looks like external array types treat the nullability of type arguments differently. Not sure if this is a roslyn bug 🤷

I realised that the same issue wouldn't occur if the external collection is a list, despite having similar code generated.

// returns a `None` annotated generic type if type is an array
 if (type.ImplementsGeneric(types.Get(typeof(IEnumerable<>)), out var enumerableIntf))
            return enumerableIntf.TypeArguments[0];

// will have the correct nullability annotation
((IArrayTypeSymbol)type).ElementType;

I'm strugglng to debug this. I can recreate the issue if I look at the generated code. Running it in debug mode generates the following code with no nullability issues 😭

I ended up creating a static public static List<String> Logger {get:} which would insert comments to the end of the final MemberDeclarationSyntax in SourceEmitter

@PaulVrugt
Copy link
Author

Awesome! Hopefully this fixes it

@github-actions
Copy link

github-actions bot commented Sep 5, 2023

🎉 This issue has been resolved in version 3.2.0-next.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

🎉 This issue has been resolved in version 3.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants