Fix issue producing metadata references in VS #73345
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| @@ -303,34 +303,24 @@ bool TryGetFileMappingFromMetadataImporter(FileKey fileKey, [NotNullWhen(true)] | |||
| FileKey fileKey, | |||
| Func<FileKey, (ModuleMetadata moduleMetadata, TemporaryStorageStreamHandle? storageHandle)> moduleMetadataFactory) | |||
| { | |||
| var (manifestModule, manifestHandle) = moduleMetadataFactory(fileKey); | |||
There was a problem hiding this comment.
the core bug was that we were not doing anything with this manifestHandle in the case where this was a multi-module assembly. I'll doc how this happened.
First, we would get the handle here. Now go to https://github.com/dotnet/roslyn/pull/73345/files#r1590487986
| moduleBuilder.Add(metadata); | ||
| storageHandles.Add(metadataStorageHandle); | ||
| } | ||
|
|
||
| if (moduleBuilder.Count == 0) |
There was a problem hiding this comment.
Second, if there were no submodules, we'd add the primary module here and the handle. But if there were submodules, then the handle for the primary module was dropped.
Now go to https://github.com/dotnet/roslyn/pull/73345/files#r1590488780
| if (assemblyDir is null) | ||
| { | ||
| moduleBuilder.Add(manifestModule); | ||
| assemblyDir = Path.GetDirectoryName(fileKey.FullPath); |
There was a problem hiding this comment.
Third: the core place i originally missed was here. Specifically, we would go in here and add the primary module to the modulebuilder, but totally lose the handle.
--
However, this code is entirely overwrought. The logic was:
- Do not add the primary items to the collections.
- if we have sub modules, add the primary items to the collections, then add hte sub modules. However, the logic here to add the primary items didn't add the handle.
- if we had no sub modules, add the primary items to the collections.
so the entire logic was convoluted, and always wanted to add the primary items as the first items. So that's just what the new logic does.
--
The reason this wasn't totally broken for everything was that case '3' is hte normal case. So for almost all users we would add the primary module and handle and be ok. It was only when we went into case '2' that things blew up.
this hit with C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework\.NETFramework\v4.7.2\System.EnterpriseServices.dll which has a submodule.
There was a problem hiding this comment.
the new logic is trivial and sane. We get the info abotu the primary module and add to the builders. Then we ge tthe info about the sub-modules, and add to hte builders. That's it :)
|
@ToddGrun please review asap. Note: this manifests as an assert in debug, and inefficiency in release. but it's still not good, and was a PITA to track down why the assert was firing. |
|
Working on a test now. |
| @@ -139,32 +138,30 @@ internal Metadata GetMetadata(string fullPath, DateTime snapshotTimestamp) | |||
| if (_metadataCache.TryGetMetadata(key, out var metadata)) | |||
| return metadata; | |||
|
|
|||
| var newMetadata = GetMetadataWorker(); | |||
| // here, we don't care about timestamp since all those bits should be part of Fx. and we assume that | |||
| // it won't be changed in the middle of VS running. | |||
There was a problem hiding this comment.
this comment moved.
| { | ||
| if (VsSmartScopeCandidate(key.FullPath)) | ||
| { | ||
| var newMetadata = CreateAssemblyMetadataFromMetadataImporter(key); |
There was a problem hiding this comment.
changed to stop passing hte full key, and just pass hte file path instead. only the file path was needed through all descendent codepaths, so this simpified things (and made it easier to test).
|
|
||
| ' We should have two handles as this assembly has two modules (itself, and one submodule for | ||
| ' System.EnterpriseServices.Wrapper.dll) | ||
| Assert.Equal(2, tuple.handles.Count) |
There was a problem hiding this comment.
This assert fails without the fix.
| Dim checksum2 = SerializerService.CreateChecksum(deserialized, cancellationToken:=Nothing) | ||
|
|
||
| ' Serializing the original reference and the deserialized reference should produce the same checksum | ||
| Assert.Equal(checksum1, checksum2) |
|
|
||
| if (!_metadataCache.GetOrAddMetadata(key, newMetadata, out metadata)) | ||
| newMetadata.Dispose(); | ||
|
|
||
| return metadata; | ||
| if (handles != null) | ||
| s_metadataToStorageHandles.Add(newMetadata, handles); |
There was a problem hiding this comment.
pulled the logic up to here for populating the caches. it simplified the lower code (imo).
| private static AssemblyMetadata CreateAssemblyMetadata( | ||
| FileKey fileKey, | ||
| Func<FileKey, (ModuleMetadata moduleMetadata, TemporaryStorageStreamHandle? storageHandle)> moduleMetadataFactory) | ||
| private static (AssemblyMetadata assemblyMetadata, IReadOnlyList<TemporaryStorageStreamHandle>? handles) CreateAssemblyMetadata( |
There was a problem hiding this comment.
made static to make this easy to unit test. this now returns both the metadta and handles to make this easy to unit tests.
...sualStudio/Core/Def/ProjectSystem/MetadataReferences/VisualStudioMetadataReferenceManager.cs
Outdated
Show resolved
Hide resolved
...sualStudio/Core/Def/ProjectSystem/MetadataReferences/VisualStudioMetadataReferenceManager.cs
Outdated
Show resolved
Hide resolved
...sualStudio/Core/Def/ProjectSystem/MetadataReferences/VisualStudioMetadataReferenceManager.cs
Outdated
Show resolved
Hide resolved
|
@jasonmalinowski For review when you get back. |
Regression introduced in #73115.