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

Unify annotations package with analyzers #948

Open
mthjones opened this issue Mar 7, 2024 · 1 comment
Open

Unify annotations package with analyzers #948

mthjones opened this issue Mar 7, 2024 · 1 comment

Comments

@mthjones
Copy link
Member

mthjones commented Mar 7, 2024

Because these are shipped as two separate packages and particularly because the analyzers are only a development dependency, it's easy for one to use the annotations without also running the analyzers which can lead to analysis holes. If we consolidated the packages, then the analyzers would flow transitively to all consumers of the annotations, making the default case the safe case. This wouldn't prevent someone going out of their way to actively disable the diagnostics, but it removes a pretty easy to set off footgun.

Example

Library A

This library references both Annotations and Analyzers.

[Immutable]
public class Foo { }

Library B

This library references Library A, so transitively references Annotations but not Analyzers.

public class Bar : Foo {
	public int Mutable { get; set; }
}

Application

This application references Library A and B, as well as both Annotations and Analyzers.

[Immutable]
public class Baz {
	// No problems here!
	public Foo Foo { get; init; }
}

Bar bar = new Bar {
	Mutable = 1
};

Baz baz = new Baz {
	Foo = bar
};

bar.Mutable += 1;  // oh no!

Open Questions

  • How should TestAnalyzers factor into this?
  • How do we transition without breaking existing dependency graphs? e.g. if we move Annotations types to a new lib, how do we avoid name conflicts?
@mthjones
Copy link
Member Author

mthjones commented Mar 7, 2024

I've tested out a simple repack of the libraries locally to verify that this behaviour works as expected. May be useful to build off:

D2L.CodeStyle.csproj

<Project Sdk="Microsoft.Build.NoTargets/3.7.56">

  <PropertyGroup>
    <TargetFramework>netstandard2.0</TargetFramework>
    <NoWarn>$(NoWarn);NU5128</NoWarn>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="D2L.CodeStyle.Annotations" Version="0.33.0" />
  </ItemGroup>

  <ItemGroup>
    <ProjectReference Include="..\D2L.CodeStyle.Analyzers\D2L.CodeStyle.Analyzers.csproj" PrivateAssets="all" ReferenceOutputAssembly="false" />
    <ProjectReference Include="..\D2L.CodeStyle.TestAnalyzers\D2L.CodeStyle.TestAnalyzers.csproj" PrivateAssets="all" ReferenceOutputAssembly="false" />
  </ItemGroup>

  <ItemGroup>
    <None Include="..\D2L.CodeStyle.Analyzers\bin\$(Configuration)\netstandard2.0\D2L.CodeStyle.Analyzers.dll" Pack="true" PackagePath="analyzers/dotnet/cs" Visible="false" />
    <None Include="..\D2L.CodeStyle.TestAnalyzers\bin\$(Configuration)\netstandard2.0\D2L.CodeStyle.TestAnalyzers.dll" Pack="true" PackagePath="analyzers/dotnet/cs" Visible="false" />
  </ItemGroup>

</Project>

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

No branches or pull requests

1 participant