tunnelvisionlabs / ReferenceAssemblyAnnotator

IL weaver to add nullability annotations to .NET reference assemblies
MIT License
71 stars 8 forks source link

netcoreapp2.1 is not annotated #50

Closed jnm2 closed 4 years ago

jnm2 commented 5 years ago
<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFrameworks>netcoreapp2.1</TargetFrameworks>
    <LangVersion>8</LangVersion>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <PropertyGroup>
    <AnnotatedReferenceAssemblyVersion>3.0.0</AnnotatedReferenceAssemblyVersion>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="TunnelVisionLabs.ReferenceAssemblyAnnotator" Version="1.0.0-alpha.90" PrivateAssets="all" />
    <PackageDownload Include="Microsoft.NETCore.App.Ref" Version="[$(AnnotatedReferenceAssemblyVersion)]" />
  </ItemGroup>

</Project>
using System.Diagnostics;

public static class C
{
    public static void M(string? x)
    {
        Debug.Assert(x is object);

        // CS8602 Dereference of a possibly null reference
        x.ToString();
    }
}
sharwell commented 5 years ago

Now that people are using this package, I'd really like to have #51 in place before this (unless it's something trivial).

jnm2 commented 5 years ago

This would be useful in https://github.com/ApiApprover/ApiApprover/blob/master/src/PublicApiGenerator.Tool/PublicApiGenerator.Tool.csproj (netcoreapp2.1) so that ! is not necessary after a string.IsNullOrEmpty check.

jnm2 commented 4 years ago

Everyone watching this issue: please try version [1.0.0-alpha.108.gaa6f8a1044] (PR https://github.com/tunnelvisionlabs/ReferenceAssemblyAnnotator/pull/59) and tell us whether or not it fixes the problem.

nuget.config
<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <packageSources>
    <add key="appveyor-referenceassemblyannotator" value="https://ci.appveyor.com/nuget/referenceassemblyannotator" />
  </packageSources>
</configuration>
jnm2 commented 4 years ago

@sharwell

Now that people are using this package, I'd really like to have #51 in place before this (unless it's something trivial).

Do you have ideas for something we can do with limited time? There are some real issues with the current package to the point that I stopped introducing it to new projects and I would prefer not to hold up the PRs that are already waiting.

sharwell commented 4 years ago

We could add some sample projects to this repository and build them after the main build, similar to this:

https://github.com/tunnelvisionlabs/antlr4cs/blob/4874e010bafe828192b6bf436a2a7d02bab161ec/build/build.ps1#L235-L287 https://github.com/tunnelvisionlabs/antlr4cs/tree/master/build/DotnetValidation

jnm2 commented 4 years ago

This is what I've been testing against. Do you think it would suffice for a start?

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

  <PropertyGroup>
    <TargetFrameworks>net35;net48;netstandard1.6;netstandard2.0;netstandard2.1;netcoreapp2.1;netcoreapp3.0;netcoreapp3.1</TargetFrameworks>
    <LangVersion>latest</LangVersion>
    <Nullable>enable</Nullable>
    <AnnotatedReferenceAssemblyVersion>3.0.0</AnnotatedReferenceAssemblyVersion>
    <NoWarn>RA1000</NoWarn>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Condition="'$(TargetFramework)' != 'net35'" Include="System.Data.SqlClient" Version="4.8.0" />
    <PackageReference Include="TunnelVisionLabs.ReferenceAssemblyAnnotator" Version="1.0.0-alpha.*" PrivateAssets="all" />
    <PackageDownload Include="Microsoft.NETCore.App.Ref" Version="[$(AnnotatedReferenceAssemblyVersion)]" />
  </ItemGroup>

</Project>
using System.Data.SqlClient;
using System.Diagnostics.CodeAnalysis;

namespace src
{
    public class Class1
    {
        public static void M(SqlConnection connection, [AllowNull] string? x)
        {
            if (!string.IsNullOrEmpty(x))
            {
                x.ToString();
            }
        }
    }
}
sharwell commented 4 years ago

It's a decent start. Keep in mind that the .props/.targets discrepancy I pointed out only impacts projects that use TargetFramework (as opposed to TargetFrameworks). To test this project with multiple targets, it could be parameterized with the parameter set by the build: <TargetFramework>$(TestFramework)</TargetFramework>.

We could make M return int to also test GetHashCode:

return EqualityComparer<string>.Default.GetHashCode(x);
jnm2 commented 4 years ago

Or the build could parameterize with /p:TargetFramework=...

I'll add that, thanks!

Separate PR okay for adding the test?

jnm2 commented 4 years ago

Rushed https://github.com/tunnelvisionlabs/ReferenceAssemblyAnnotator/pull/60