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

"An item with the same key has already been added" with metadata 10.0.19041.202-preview #273

Closed
Ssiws opened this issue May 21, 2021 · 19 comments
Labels
bug Something isn't working

Comments

@Ssiws
Copy link

Ssiws commented May 21, 2021

Actual behavior

After adding reference to "Microsoft.Windows.SDK.Win32Metadata" Version "10.0.19041.202-preview" the SourceGenerator fails:

CSC : warning CS8785: Generator 'SourceGenerator' failed to generate source. It will not contribute to the output and compilation errors may occur as a result.
Exception was of type 'ArgumentException' with message 'An item with the same key has already been added. Key: JsCreateContext'

Expected behavior

Adding this reference
<PackageReference Include="Microsoft.Windows.SDK.Win32Metadata" Version="10.0.19041.202-preview" /> should not cause this warning.

Repro steps

  1. NativeMethods.txt content:
CredUIPromptForWindowsCredentialsW
  1. NativeMethods.json content (if present):
    --

  2. Any of your own code that should be shared?

From csproj:

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

  <PropertyGroup>
    <OutputType>WinExe</OutputType>
    <TargetFramework>net5.0-windows</TargetFramework>
    <UseWPF>true</UseWPF>
    <IncludePackageReferencesDuringMarkupCompilation>true</IncludePackageReferencesDuringMarkupCompilation>
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <LangVersion>9</LangVersion>
  </PropertyGroup>

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

  <ItemGroup>
    <Resource Include="NativeMethods.txt" />
  </ItemGroup>

</Project>

Context

  • CsWin32 version: 0.1.422-beta
  • Win32Metadata version (if explicitly set by project): 10.0.19041.202-preview
  • Target Framework: net5.0-windows
  • LangVersion (if explicitly set by project): 9
@Ssiws Ssiws added the bug Something isn't working label May 21, 2021
@AraHaan
Copy link
Contributor

AraHaan commented May 21, 2021

There is a newer preview release available in the feed at the bottom of the readme. At the end of today the new fixes should get pushed to there to ensure it works and then later pushed to nuget.org.

@Ssiws
Copy link
Author

Ssiws commented May 21, 2021

Thanks for pointing this out. I did some tests with 0.1.474-beta (latest) but this build is not working at all, the generator is not called by dotnet build so I end up with The type or namespace name 'Sdk' does not exist in the namespace[...].

Then, I tested with 0.1.454-beta, and this version shows the same behavior as 10.0.19041.202-preview

@AraHaan
Copy link
Contributor

AraHaan commented May 21, 2021

Odd @AArnott mind investigating further.

@AArnott
Copy link
Member

AArnott commented May 21, 2021

We don't yet support updating metadata beyond what CsWin32 was tested with. This is because the metadata keeps changing in breaking ways. You can always try it, but if it breaks, you have to stick to the default metadata and wait till we ship an update, which as @AraHaan says, we just recently did (so far, just to our nightly CI feed).

I did some tests with 0.1.474-beta (latest) but this build is not working at all, the generator is not called by dotnet build so I end up with The type or namespace name 'Sdk' does not exist in the namespace[...].

Good. I'm glad you tried it. But I suspect your conclusion that "this build is not working at all" is incorrect. In fact the new version doesn't generate anything into the Microsoft.Windows.Sdk namespace any more. Instead, look for the Windows.Win32 namespace. There will be subnamespaces with many of the the actual generated interop types as well.

@AArnott AArnott closed this as completed May 21, 2021
@jnm2
Copy link
Contributor

jnm2 commented May 21, 2021

In fact the new version doesn't generate anything into the Microsoft.Windows.Sdk namespace any more. Instead, look for the Windows.Win32 namespace. There will be subnamespaces with many of the the actual generated interop types as well.

What's really unfortunate in both the current release and current preview of VS is that the normal fix to add a using directive is not offered for generated types. So I had to manually type in using Windows.Win32; once I saw you say this, and I still don't know why HWND isn't resolving, so I'll have to poke around and guess some more.

@jnm2
Copy link
Contributor

jnm2 commented May 21, 2021

Was it using Windows.Win32.Graphics;?
Nope.
Was it using Windows.Win32.System;?
Nope.
Was it using Windows.Win32.UI;?
Nope.

Wait, no other options are shown in IntelliSense.
Oh. Maybe there are nested namespaces even deeper.

Is it using Windows.Win32.System.SystemServices;?
Nope.
Is it using Windows.Win32.UI.WindowsAndMessaging;?
🎉

That got HWND. HCURSOR is still not found yet.

Is the learning path always going to be a namespace guessing game until Roslyn starts offering automatic fixes?

@jnm2
Copy link
Contributor

jnm2 commented May 21, 2021

Is there a way to opt out of namespaces?

In a single file of mine:
HWND is in Windows.Win32.UI.WindowsAndMessaging.
HCURSOR is in Windows.Win32.UI.MenusAndResources.
POINT is in Windows.Win32.UI.DisplayDevices.

And BOOL is in Windows.Win32.System.SystemServices, even though I already have Windows.Win32 imported. This is a lot.

I actually think I searched every possible namespace and I haven't found one that contains CURSORINFO_flags yet. Looping back to see what I messed up. Ah, the capitalization changed. Again, something VS usually has a light bulb fix for.

@AArnott
Copy link
Member

AArnott commented May 21, 2021

What's really unfortunate in both the current release and current preview of VS is that the normal fix to add a using directive is not offered for generated types.

Ya, I discovered this yesterday as well. Most unfortunate. I filed a feedback ticket to ask that they fix that:
https://developercommunity.visualstudio.com/t/C-Add-using-refactoring-not-offered-f/1428570
I think that ticket is "internal" but I've asked the powers that be to make it public so folks can vote it up.

Is the learning path always going to be a namespace guessing game until Roslyn starts offering automatic fixes?

There is a better way: set your caret on the unbound identifier and press Ctrl+T, which activates Go To All. That will jump to the generated symbol so you can see what namespace it's defined in.

Finally, in the metadata repo and team discussions we are looking for ways to align APIs into fewer namespaces to reduce the pain.

@jnm2
Copy link
Contributor

jnm2 commented May 21, 2021

@AArnott I wonder if filing directly at https://github.com/dotnet/roslyn might get a faster response. I've seen some VS feedback items come through to there that seemed to have been in the system a long time before appearing on GitHub, though my sample size is small and maybe I didn't understand what I was seeing.

But also, what if I don't like this change? Is there anything I can do besides waiting for global using directives to come in C# 10? Or do you think the metadata discussions will make this nice enough that I'll have fewer arbitrary-feeling using directives? (E.g. one namespace per API DLL name seems fine.)

image

image

@mikebattista
Copy link
Contributor

We're in the middle of refactoring the namespaces. This is the first update where C#/Win32 is surfacing them so there is some dirty laundry here. From Andrew, he no longer can generate everything in one namespace for various technical reasons, which is driving this change, so I'm not sure an opt-out would be possible. Andrew?

We need to move common types like the ones you mentioned (HANDLE, HWND, string types, etc.) up to a central namespace like Windows.Win32.Foundation (to align with the WinRT namespaces) or possibly the root at Windows.Win32. So the hope would be you add Windows.Win32 and/or Windows.Win32.Foundation as table stakes, and then add the individual namespaces you need which should be mostly self-contained. If lack of this is causing pain, we can prioritize that work.

Please do file namespace issues in the win32metadata repo. We're actively refactoring things. If you want to help contribute even further, see https://github.com/microsoft/win32metadata/blob/master/CONTRIBUTING.md#Namespaces for how to file your feedback as PRs.

@AArnott
Copy link
Member

AArnott commented May 21, 2021

no longer can generate everything in one namespace for various technical reasons, which is driving this change, so I'm not sure an opt-out would be possible.

The trouble is the Win32 API reuses the same interface or struct name across header files for conflicting purposes. In the metadata we express this as types across namespaces to avoid the collision. In previous versions of CsWin32, we "dealt" with this by non-deterministically choosing one type to be the winner and suppressing generation of the loser(s). Obviously that was a hack and wouldn't work for folks that actually need to use those types. Namespaces are the only way to resolve such conflicts.

For small scoped CsWin32 generation, I wholeheartedly agree that one namespace is more convenient. We are however looking at a couple other goals that must influence this:

  1. We want to expose the full CsWin32 projection as one or more NuGet packages so folks can consume rather than generate the APIs. This is particularly interesting for non-C# .NET developers like VB.NET and F# where source generators are not an option.
  2. We want to expose the full API set by default when .NET SDK projects specify that they are targeting Windows in some future version of the .NET SDK.
    In both of these cases, introducing many thousands of APIs into just one namespace may become more bothersome than convenient.

For the partial generation case that is CsWin32 today, we also want to enable folks to generate islands of related APIs by namespace the way you can by module today (#259), which would make most sense if the APIs generated actually were in that namespace.

So, we are thinking that generating into multiple namespaces is the right path forward. But organizing what was a flat list of many thousands of APIs into a user-friendly tree of namespaces is hard. We will drive the improve the experience on all sides: this generator, the C# lang svc, and in the metadata repo where the namespaces are actually defined.

@jnm2
Copy link
Contributor

jnm2 commented May 21, 2021

If lack of this is causing pain, we can prioritize that work.

It makes me hesitant to move projects to newer CsWin32 releases until that work is done, but I'm not blocked or feeling like you should shift priorities for my sake. I wanted to give early feedback, and it looks like you all have this well thought out and that's all that matters to me. 👍

@AArnott
Copy link
Member

AArnott commented May 21, 2021

It makes me hesitant to move projects to newer CsWin32 releases until that work is done

That would be a loss for us, because you've provided a lot of great feedback and I hope that keeps coming.
Perhaps we can add an option to NativeMethods.json whereby you can ask for a "flattened" namespace to generate code into. That should always work--until you want APIs with conflicting names.

@jnm2
Copy link
Contributor

jnm2 commented May 21, 2021

If that works easily enough for you, that would be awesome! I'll see my use of it as a temporary measure to avoid the intermediate namespace churn and the tedium of working around the VS bug.

@mikebattista
Copy link
Contributor

I'm supportive of an opt-out if that's possible for the simple source generation case. It's unfortunate the tooling doesn't yet help here. +1 to everything you said Andrew about the other goals and scenarios outside of source generation that we're trying to balance as well.

I'll work on the common foundation namespace. Based on the early feedback, lack of that seems to be the biggest pain point.

@AraHaan
Copy link
Contributor

AraHaan commented May 21, 2021

I think there should be a way to filter out regenerating the same type in multiple namespaces if that type matches 100% with the version inside of another namespace. That would also simplify confusions like stated above as well too.

(I am looking at you HGLOBAL that seems to not be stated above as well 👀)

@AArnott
Copy link
Member

AArnott commented May 21, 2021

Tip: the https://developercommunity.visualstudio.com/t/C-Add-using-refactoring-not-offered-f/1428570 ticket is now public so you can vote it up.

@AArnott
Copy link
Member

AArnott commented May 21, 2021

I think there should be a way to filter out regenerating the same type in multiple namespaces if that type matches 100% with the version inside of another namespace. That would also simplify confusions like stated above as well too.

Where such types exist, that would be great feedback to send to the win32metadata repo.

(I am looking at you HGLOBAL that seems to not be stated above as well 👀)

I just searched 10.0.19041.202-preview of the metadata and don't see HGLOBAL anywhere (let alone two namespaces). Where should I be looking, @AraHaan?

@AraHaan
Copy link
Contributor

AraHaan commented May 21, 2021

I do not think it is part of the metadata yet. I could be wrong though. But there are APIs that use it for sure.

AArnott added a commit that referenced this issue Jun 12, 2024
Enable ShallowClone when performing insertion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants