zompinc / sync-method-generator

Generates a synchronized version of an async method
MIT License
46 stars 4 forks source link

Generator stops generating code with multi-project solution #76

Open persn opened 4 months ago

persn commented 4 months ago

This problem is a little complicated to setup but I think I was able to find a minimal reproduction, although I can't even begin to guess what could be wrong here. It specifically happens when using 3+ projects with InternalsVisibleTo and SYNC_METHOD_GENERATOR_DISABLE_ATTRIBUTE_GENERATION. The code generator doesn't generate any code in Project 3, there's no Foo3(). It happens completely silently and I have to try and reference the method or add it as a interface contract to force a build error.

Project 1

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>netstandard2.0</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Zomp.SyncMethodGenerator" Version="1.3.44">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
  </ItemGroup>

</Project>

Project 2

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>netstandard2.0</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Zomp.SyncMethodGenerator" Version="1.3.44">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
  </ItemGroup>

  <ItemGroup>
    <ProjectReference Include="..\TestInternals1\TestInternals1.csproj" />
  </ItemGroup>

</Project>
using System;
using System.Runtime.CompilerServices;

[assembly: InternalsVisibleTo("TestInternals3")]

Project 3

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>netstandard2.0</TargetFramework>
    <DefineConstants>$(DefineConstants);SYNC_METHOD_GENERATOR_DISABLE_ATTRIBUTE_GENERATION</DefineConstants>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Zomp.SyncMethodGenerator" Version="1.3.44">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
  </ItemGroup>

  <ItemGroup>
    <ProjectReference Include="..\TestInternals2\TestInternals2.csproj" />
  </ItemGroup>

</Project>
using System;
using System.Threading.Tasks;

namespace TestInternals3
{
    public interface IFoo // The build finishes without any complaints so this interface will force a compiler error if it fails to generate the sync version of the method
    {
        void Foo();
        Task FooAsync();
    }

    public partial class Class1 : IFoo
    {
        [Zomp.SyncMethodGenerator.CreateSyncVersion(OmitNullableDirective = true)]
        public Task FooAsync() => Task.CompletedTask;
    }
}
persn commented 4 months ago

image

virzak commented 4 months ago

@persn,

Why not just drop SYNC_METHOD_GENERATOR_DISABLE_ATTRIBUTE_GENERATION from project 3?

Project 3 marks FooAsync with CreateSyncVersionAttribute, but it is disabled for both project 3 and 2. It cannot access that attribute in project 1, since <InternalsVisibleTo Include="TestInternals3" /> is missing.

On my side CreateSyncVersionAttribute in project 3 isn't even highlighted, which is what I would expect. When you hit F12 in Visual Studio, where does it navigate?

persn commented 4 months ago

What do you mean it is disabled for project 2? I didn't actively toggle it to do so. The whole point of disabling it in project 3 is because it conflicts with the generated attribute in project 2.

A detail worth adding in is if I comment out the ProjectReference in Project 2, then everything works as expected

persn commented 4 months ago

I see a mistake in Project 1 btw, but I think it should be obvious, it's not supposed to have a a ProjectReference, I'll update it

virzak commented 4 months ago

Here is my configuration of your project.

Issue-76.zip

See how project 3 doesn't use SYNC_METHOD_GENERATOR_DISABLE_ATTRIBUTE_GENERATION

Is that an acceptable solution?

persn commented 4 months ago

No not really, seems you just made InternalsVisible from Project1 to Project2 instead and that works for some bizarre reason. The problem is that where to use InternalsVisibleTo is a strategic decision, because otherwise you're leaking out implementation details wherever you're not supposed to. Usually InternalsVisibleTo are used on test projects which is safe because they're not distributed anywhere. In this example you can assume Project3 is a test project, and the other 2 are assembleis that are distributed to 3rd parties.

Besides.. If anyone else run into this issue, how on earth are they supposed to figure out that they can solve it this way? It's not at all intuitive, and I'm pretty sure that once I start adding the analyzer to even more project in our 100+ projects big monolith, then this "solution" will not be so good anymore.

I'm afraid the best workaround right now would be to simply not use the code generator at all for Project3.

virzak commented 4 months ago

Issue-76.zip

How about this now?

This compiles fine for me

persn commented 4 months ago

Here you just removed the preprocessor directive from Project 3, which means it'll produce a compiler warning when it has 2 versions of the attribute to choose from. However the whole reason this preprocessor directive was introduced was because for solutions with warning as error it'll error. Add a <TreatWarningsAsErrors>true</TreatWarningsAsErrors> to your Directory.Build.props, it's unlikely to keep compiling.

Also a lot of devs doesn't want compiler warnings in their build, once you start allowing one, they tend to pile up

I'm afraid the better solution is still to not use the code generator for the third project.

persn commented 4 months ago

This blog discusses how to solve attribute generation https://andrewlock.net/creating-a-source-generator-part-7-solving-the-source-generator-marker-attribute-problem-part1/

Right now this generator uses solution 1, but that has problems for us which is also discussed in the article.

It seems we can get around the current problem by using solution 2, defining our own attribute and disabling the generator everywhere we use it appears to work.

Hower it's a little tedious, and I think in the long run the generator would be best off implementing solution 3 as discussed in the blog.

virzak commented 4 months ago

I think an obvious solution would be to allow a user define an additional macro.

Currently we have:

#if !SYNC_METHOD_GENERATOR_DISABLE_ATTRIBUTE_GENERATION

If your csproj contains a line like

<PropertyGroup>
  <AdditionalSyncMethodGeneratorDisableAttributes>PROJ_2_DISABLE_ATTRIBUTE_GENERATION</AdditionalSyncMethodGeneratorDisableAttributes>
</PropertyGroup>

The generated code would look like this:

#if !SYNC_METHOD_GENERATOR_DISABLE_ATTRIBUTE_GENERATION && !PROJ_2_DISABLE_ATTRIBUTE_GENERATION

... or even better: the property could be named SyncMethodGeneratorDisableAttribute. Then the resulting generation could be:

#if !PROJ_2_DISABLE_ATTRIBUTE_GENERATION

What do you think?

virzak commented 3 months ago

Actually that won't work since "RegisterPostInitializationOutput" only generates static string which has no access to the compilation object.