-
Notifications
You must be signed in to change notification settings - Fork 94
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
Comments
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. |
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 Then, I tested with |
Odd @AArnott mind investigating further. |
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).
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 |
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 |
Was it Wait, no other options are shown in IntelliSense. Is it 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? |
Is there a way to opt out of namespaces? In a single file of mine: And BOOL is in Windows.Win32.System.SystemServices, even though I already have Windows.Win32 imported. This is a lot.
|
Ya, I discovered this yesterday as well. Most unfortunate. I filed a feedback ticket to ask that they fix that:
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. |
@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.) |
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. |
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:
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. |
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. 👍 |
That would be a loss for us, because you've provided a lot of great feedback and I hope that keeps coming. |
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. |
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. |
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 👀) |
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. |
Where such types exist, that would be great feedback to send to the win32metadata repo.
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? |
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. |
Enable ShallowClone when performing insertion
Actual behavior
After adding reference to "Microsoft.Windows.SDK.Win32Metadata" Version "10.0.19041.202-preview" the SourceGenerator fails:
Expected behavior
Adding this reference
<PackageReference Include="Microsoft.Windows.SDK.Win32Metadata" Version="10.0.19041.202-preview" />
should not cause this warning.Repro steps
NativeMethods.txt
content:NativeMethods.json
content (if present):--
Any of your own code that should be shared?
From csproj:
Context
LangVersion
(if explicitly set by project): 9The text was updated successfully, but these errors were encountered: