Conversation
The portable PDB writing code was not properly encoding external aliases. They were being ignored since they aren't included in the `CommonReferenceManager` as the code expected. Convertd to a more direct way of grabbing the `PEReader` value and properly encoded the modules
|
@dotnet/roslyn-compiler PTAL |
src/Compilers/Core/Portable/PEWriter/MetadataWriter.PortablePdb.cs
Outdated
Show resolved
Hide resolved
| Add(Permutate(new CommandInfo("hw.cs /target:exe /debug:embedded", "test.exe", null))); | ||
| Add(Permutate(new CommandInfo("lib1.cs /target:library /debug:embedded", "test.dll", null))); | ||
| Add(Permutate(new CommandInfo("lib2.cs /target:library /r:SystemRuntime=System.Runtime.dll /debug:embedded", "test.dll", null))); | ||
| Add(Permutate(new CommandInfo("lib3.cs /target:library /r:SystemRuntime1=System.Runtime.dll /r:SystemRuntime2=System.Runtime.dll /debug:embedded", "test.dll", null))); |
There was a problem hiding this comment.
/r:SystemRuntime2=System.Runtime.dll [](start = 104, length = 36)
Would it make sense to reference two different versions of System.Runtime.dll, so it's clear that references in the rebuilt binary are to the expected versions?
There was a problem hiding this comment.
The case I'm interested in testing here is when it refers to the same DLL
| return metadataReference.Properties.Aliases.Length == 1 | ||
| ? metadataReference.Properties.Aliases[0] | ||
| : null; | ||
| } |
There was a problem hiding this comment.
Consider replacing with metadataReference.Properties.Aliases.SingleOrDefault().
| imageSize, | ||
| name, | ||
| mvid, | ||
| alias == "global" ? null : alias, |
There was a problem hiding this comment.
does this constant live anywhere in the compiler that we can reference from here?
There was a problem hiding this comment.
Not that I could find and I couldn't find a place that made sense as a place to define so that it would be usable across all the layers of the compiler.
| public readonly FileInfo FileInfo; | ||
| public readonly Guid Mvid; | ||
| public readonly ImmutableArray<string> ExternAliases; | ||
| public readonly string? ExternAlias; |
There was a problem hiding this comment.
why did we delay splitting the alias names?
There was a problem hiding this comment.
My guess is that the original implementation attempted to represent extern aliases in the same way that MSBuild does. Essentially there is one reference with a collection of aliases. In reality though the compiler ends up representing it more as a 1-1 mapping by the time you get to the command line. This change better reflects the compiler representation and is more suited to reubild scenarios.
| properties: new MetadataReferenceProperties( | ||
| kind: MetadataImageKind.Assembly, | ||
| aliases: reference.ExternAliases, | ||
| aliases: reference.ExternAlias is null ? ImmutableArray<string>.Empty : ImmutableArray.Create(reference.ExternAlias), |
There was a problem hiding this comment.
Isn't the property ExternAlias of type string, meaning this expression will create an array of a single alias even if multiple comma-separated aliases are present?
There was a problem hiding this comment.
This code will maintain the pattern of a 1:1 relationship between extern alias and the reference. That is how the compiler actually represents extern aliases internally. The fact that the property back on MetadataReference has a collection of aliases is a red herring. It may be useful in API driven scenarios (have not checked) but in the command line scenario it is always a 1:1 relationship.
It is a very confusing detail and the majority of my time in this change was understanding this relationship.
The portable PDB writing code was not properly encoding external
aliases. They were being ignored since they aren't included in the
CommonReferenceManageras the code expected. Convertd to a more directway of grabbing the
PEReadervalue and properly encoded the modules