Emit warning/error when System.Memory reference is missing but required by emitted code

andrekoehler commented 3 years ago

Actual behavior

As soon as I add 'CharLower' to my NativeMethods.txt I get the following build error:

Error   CS0246  The type or namespace name 'Span<>' could not be found (are you missing a using directive or an assembly reference?)
C:\dev\_tests\ConsoleApp3\Microsoft.Windows.CsWin32\Microsoft.Windows.CsWin32.SourceGenerator\PWSTR.g.cs

Other methods like 'MFStartup' which don't take strings work fine.

Expected behavior

The project should compile without errors even if using .NET Framework v4.8 / netstandard2.0

Repro steps

Create new .NET Framework 4.8 project or create .NET 5.0 project and change target framework to 'net48' or 'netstandard2.0'.
NativeMethods.txt content:
```
CharLower
```
NativeMethods.json content (if present): does not exist
Build project

Context

CsWin32 version: 0.1.422-beta
Win32Metadata version (if explicitly set by project): default
Target Framework: net48 or netstandard2.0
LangVersion: 8.0

jnm2 commented 3 years ago

A workaround might be to add this to your csproj:

    <PackageReference Include="System.Memory" Version="4.5.4" />

andrekoehler commented 3 years ago

@jnm2 Thank you, your workaround fixed the issue. I wonder why this is necessary, because the CsWin32.nuspec file clearly specifies the dependency: https://github.com/microsoft/CsWin32/blob/2216f3e461adb79d1274b07414f3951e3324ef95/src/Microsoft.Windows.CsWin32/Microsoft.Windows.CsWin32.nuspec#L20-L22

AArnott commented 3 years ago

It shouldn't be necessary. Your repro steps didn't repro the problem for me:

I'm glad you have a workaround, but I think there must be something else at play that makes it required for you.

andrekoehler commented 3 years ago

@AArnott This is really weird. What version of .NET SDK and Visual Studio do you use? Screenshot (191)

C:\Users\andre>dotnet --info
.NET SDK (reflecting any global.json):
 Version:   5.0.201
 Commit:    a09bd5c86c

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.19042
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\5.0.201\

Host (useful for support):
  Version: 5.0.4
  Commit:  f27d337295

.NET SDKs installed:
  5.0.101 [C:\Program Files\dotnet\sdk]
  5.0.103 [C:\Program Files\dotnet\sdk]
  5.0.201 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.All 2.1.26 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.26 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.13 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.4 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.1.26 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.13 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.4 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.1.13 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.4 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

C:\dev\_tests\ClassLibrary2>dotnet build
Microsoft (R) Build Engine version 16.9.0+57a23d249 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

  Determining projects to restore...
  All projects are up-to-date for restore.
C:\dev\_tests\ClassLibrary2\Microsoft.Windows.CsWin32\Microsoft.Windows.CsWin32.SourceGenerator\PWSTR.g.cs(41,18): error CS0246: The type or namespace name 'Span<>' could not be found (are you missing a using directive or an assembly reference?) [C:\dev\_tests\ClassLibrary2\ClassLibrary2.csproj]

Build FAILED.

C:\dev\_tests\ClassLibrary2\Microsoft.Windows.CsWin32\Microsoft.Windows.CsWin32.SourceGenerator\PWSTR.g.cs(41,18): error CS0246: The type or namespace name 'Span<>' could not be found (are you missing a using directive or an assembly reference?) [C:\dev\_tests\ClassLibrary2\ClassLibrary2.csproj]
    0 Warning(s)
    1 Error(s)

Time Elapsed 00:00:05.67

andrekoehler commented 3 years ago

I just reproduced the same issue on two other computers: My HTPC (Win10 Pro) and my Laptop (Win10 Home). Both never had Visual Studio or .NET SDK installed before. I installed .NET SDK v5.0.5 on both of them and then ran dotnet build after extracting the following zip file: ClassLibrary2.zip

So this issue is really not on my side.

jnm2 commented 3 years ago

That repros for me. The issue comes from <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> inside the package reference. This is boilerplate that .NET tooling pushes hard for packages that are development dependencies. I always remove it:

  <ItemGroup>
    <PackageReference Include="Microsoft.Windows.CsWin32" Version="0.1.422-beta" PrivateAssets="all" />
  </ItemGroup>

After that, you get error CS8805: Program using top-level statements must be an executable. because the project is missing <OutputType>.

andrekoehler commented 3 years ago

Oh, ok.

I added the CsWin32 package reference using the Visual Studio "Manage NuGet Packages..." Wizard. I did not modify it manually.

    <PackageReference Include="Microsoft.Windows.CsWin32" Version="0.1.422-beta">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>

jnm2 commented 3 years ago

Yes, that's part of what I mean when I say the .NET tooling pushes this hard. I'm not saying it's your fault. It's a new thing to overcome.

jnm2 commented 3 years ago

@AArnott It would be my personal preference to be able to opt out of (or into!) the System.Memory package and have friendly overloads fall back to the next best thing if you didn't happen to want the extra runtime dependency. If it was an opt-in model, this issue just disappears.

andrekoehler commented 3 years ago

I also repro the issue when copying the PackageReference string from the NuGet website, which (compared to Visual Studio) does not include the buildtransitive flag:

<PackageReference Include="Microsoft.Windows.CsWin32" Version="0.1.422-beta">
  <PrivateAssets>all</PrivateAssets>
  <IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
</PackageReference>

AArnott commented 3 years ago

If it was an opt-in model, this issue just disappears.

It transforms from a build error that led the user to add the missing package reference to one where the user subtly gets sub-optimal code when they were perfectly willing to add the package reference.

jnm2 commented 3 years ago

@AArnott I thought of that. A suggestion in the error list or at the call site might help. Anyway, just mentioning it as a potential solution to this issue.

AArnott commented 3 years ago

Yes, I think a warning/error added to the compilation when the reference is missing (and required or optional) is a good idea.

AArnott commented 3 years ago

The problem is the IncludeAssets metadata leaving out compile. That does exactly what it's supposed to (leaving System.Memory off as a reference) but it isn't what we want. I'll explore why VS is omitting this by default.

jnm2 commented 3 years ago

I asked a knowledgeable community member and they said they couldn't make it work and gave up: https://gitter.im/dotnet/csharplang?at=60707e8c97cf50674650c653

microsoft / CsWin32