@jmarolf There's one thing I still don't understand about this though: for projects that have no GUID, isn't one made up for them for registration for IVsSolution? If there's no temporary GUID, how are we supposed to report diagnostics for these projects?

Is the inability to give a GUID for this project just a temporary thing, where the IVsHierarchy can't answer the question right away?

@jasonmalinowski I talked this over with @tmeschter who I consider the expert on how all this works. the project system does not generate GUIDs as far as we can tell. Essentially GUID have never been stable or guaranteed to exists. Error reporting might fail for them, but we still need to be able to open these projects.

If there's no temporary GUID, how are we supposed to report diagnostics for these projects?

What happens today if the user copy/pasted the same GUID into all of their projects?

I think the answer is things don't work very well. But this isn't as bad as not being able to open the project at all.

My impression is the project system generates a new GUID in that case. To be clear: I don't think our request for the GUID is for the file GUID -- we want the GUID that's the IVsSolution GUID. That may be seeded by the file contents but that's not guaranteed and definitely not something VS assumes.

@jmarolf @jasonmalinowski I took a closer look at the CSProj project system. It doesn't query the property from MSBuild nor does it generate a new one if it doesn't have one; rather it is just using whatever the shell has told it to use.

It's entirely possible you're just querying for the project GUID too early. Prior to #35746 the language service handled this by just setting its copy of the GUID to Guid.Empty; after #35746 it throws.

I don't understand the "too early"part. Are you saying that IVsHierarchy is not assigned a project GUID in the shell when the project system creates the project?

I guess I'm mostly worried is @tmat's change is generally moving from a pattern where we held IVsHierarchy (and then might query the GUID later when the project system does have it) to holding the GUID directly and early; we'll need to figure out better what the pattern is on our side. Thanks @tmeschter for the extra info!

@jasonmalinowski In other words previously the features that used project GUID were non-deterministic. There was no guarantee anywhere that the guid is available at the point when it's being read, so the feature just silently failed randomly?

Hard to say if it was actually non-deterministic. If the project system just called us before registering into IVsSolution, but didn't yield the UI thread, nothing else would have ran on the UI thread later querying it.

Right. The problem actually is that we stored the empty guid to the project to guid map and never updated it. So features that needed the guild didn't work.

Seems like this change uncovered a bug that was there before. We need to be notified when the GUID is set for a project.

The PR test is failing because later changes were made that depend on the change being reverted. It might be simpler to just fix the particular issue. I'll update another PR that's fixing a different issue with the change: #36461

@jasonmalinowski In other words previously the features that used project GUID were non-deterministic. There was no guarantee anywhere that the guid is available at the point when it's being read, so the feature just silently failed randomly?

@tmat that was my understanding. Previously things worked by accident not design. I agree this is a bug. The higher order bit for me is not breaking project load for 16.2. Figuring out how project guid lifetimes (?) should be communicated is something we can resolve in a subsequent pr

Fix: 756d9c2

Let's address the specific issue instead of reverting the whole change.

I'll leave that up @jasonmalinowski. But even if we generate a new GUID on the project system side if its empty @tmeschter tells me we do nothing if there are duplicate GUIDs in a solution. The idea of unique GUIDs in the project system is not a sound concept as far as I can tell. We would need more testing to prove that just the change in 756d9c2 is sufficient.

tells me we do nothing if there are duplicate GUIDs in a solution.

Doesn't this mean we then can't do things like report errors or whatever? Where does the project system ultimately register the GUID with IVsSolution? I'd be curious to look at the code.

@jasonmalinowski The project system (by which I mean specifically csproj.dll) doesn't register the GUID with the IVsSolution. Instead, some time after project creation the solution tells the project what GUID it should use by calling IVsHierarchy::SetGuidProperty, e.g. m_pHierarchy->SetGuidProperty(VSITEMID_ROOT, VSHPROPID_ProjectIDGuid, newGuid).

Interesting. I just tested C# WPF project and it had the GUID at the time the project was constructed. VB WPF did not. Can/do you delay calling Roslyn create project until you have the GUID?

@tmeschter: Instead, some time after project creation the solution tells the project what GUID it should use by calling IVsHierarchy::SetGuidProperty, e.g. m_pHierarchy->SetGuidProperty(VSITEMID_ROOT, VSHPROPID_ProjectIDGuid, newGuid).

Ah, interesting. And @jmarolf was chatting with me yesterday and implied this does come from the project GUID in the file; we've confirmed that the file isn't involved here?

I think what happens is that when the project is added to the solution, the GUID is read from the project file and put into the solution file. If it does not have a GUID, the solution creates one for it and puts that into the solution file. When the project is opened the solution asks the project system to create the hierarchy for the project, and then later sets the GUID using whatever was previously written to the solution file

So in many cases the GUID ultimately comes from the project file, but it doesn't have to.