Skip to content

Support generics in analyzer - #1854

Closed
treziac wants to merge 1 commit into
MessagePack-CSharp:masterfrom
treziac:master
Closed

Support generics in analyzer#1854
treziac wants to merge 1 commit into
MessagePack-CSharp:masterfrom
treziac:master

Conversation

@treziac

@treziac treziac commented Jun 24, 2024

Copy link
Copy Markdown

fixes #1853

@treziac

treziac commented Jun 24, 2024

Copy link
Copy Markdown
Author

Must still improve: won't work in the example when using Foo<Thing>, with Thing still an other object not properly marked with attributes, will improve it.

I didn't check the impact on the developer branch with v3, can look at it if requested

@treziac treziac changed the title Support generics in analyzer Draft: Support generics in analyzer Jun 24, 2024
@treziac
treziac marked this pull request as draft June 24, 2024 11:31
@treziac treziac changed the title Draft: Support generics in analyzer Support generics in analyzer Jun 24, 2024
@AArnott

AArnott commented Jun 24, 2024

Copy link
Copy Markdown
Collaborator

Thanks for contributing.
I'm glad you're already thinking of the impact on the develop branch. In fact I urge you to rebase now and target the develop branch initially, because that's where a bunch of this work is happening. We won't take the PR if it only works for master and creates merge conflicts in develop. But I'd take a PR that only targets develop.
After it works for develop, we can rebase the change back to master to see if it comes cleanly, and if it does, we can merge it there too. Or you can author the fix for both branches individually -- that works too. But develop is very much the priority right now.

@treziac

treziac commented Jun 26, 2024

Copy link
Copy Markdown
Author

Unfortunately I'm not sure to understand how analyzer are supposed to work on develop - I don't manage to have it running on sandbox project and am less familiar with SourceGenerator.
I didn't found anything in the migration markdown which explains it. For example this code

// Copyright (c) All contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using MessagePack;

namespace ConsoleApp1;

public class Program
{
    [MessagePackObject]
    public class A
    {
        public int Member { get; set; }
    }

    public static void Main(string[] args)
    {
        var bytes = MessagePackSerializer.Serialize(new A { Member = 10 });
        var b = MessagePackSerializer.Deserialize<A>(bytes);

        Console.WriteLine(b.Member == 10);
    }
}
<Project Sdk="Microsoft.NET.Sdk">

    <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFramework>net8.0</TargetFramework>
        <ImplicitUsings>enable</ImplicitUsings>
        <Nullable>enable</Nullable>
    </PropertyGroup>

    <ItemGroup>
      <ProjectReference Include="..\..\src\MessagePack.Analyzers\MessagePack.Analyzers.csproj"
                        OutputItemType="Analyzer"
                        ReferenceOutputAssembly="false" />
      <ProjectReference Include="..\..\src\MessagePack\MessagePack.csproj" />
    </ItemGroup>

</Project>

I don't get any warning/error at compilation for missing Key/IgnoreMember attributes, which would then crash at runtime.

@AArnott

AArnott commented Jun 26, 2024

Copy link
Copy Markdown
Collaborator

Your last repro works fine on v3:
image

The repro in your original issue however may still be a problem. I'm writing a test now to see.

@AArnott

AArnott commented Jun 26, 2024

Copy link
Copy Markdown
Collaborator

#1859 adds the test and fixes the bug for v3.
The fix doesn't readily port to v2, but I suspect logically it does if you want to update your PR with the fix I made for v3. Otherwise, let's close this PR.

@AArnott AArnott closed this Jul 19, 2024
@AArnott AArnott added this to the v2.5 milestone Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MsgPack003 is not reported on generic typed members

2 participants