Fix null reference error on ResolveAssemblyReference

[sfoslund]

Context

Null reference error when ReferenceVersions are not set, introduced in #5990

Testing

Bug repros with dotnet/sdk#15430. These changes were tested with that PR and fix the issue.

[benvillalobos]

Would it be better to pass an empty string rather than null if ReferenceVersion is null?

[sfoslund]

It doesn't seem to effect the outcome, does it? Either way there isn't a value for the metadata on this item.

[benvillalobos]

I wasn't sure how it would ultimately was used, so I looked into it. Looks like it null checks anyway. LGTM

            /// <summary>
            /// Returns the escaped value of the requested metadata name.
            /// </summary>
            string ITaskItem2.GetMetadataValueEscaped(string metadataName)
            {
                ErrorUtilities.VerifyThrowArgumentNull(metadataName, nameof(metadataName));

                string metadataValue = null;

                if (FileUtilities.ItemSpecModifiers.IsDerivableItemSpecModifier(metadataName))
                {
                    // FileUtilities.GetItemSpecModifier is expecting escaped data, which we assume we already are.
                    // Passing in a null for currentDirectory indicates we are already in the correct current directory
                    metadataValue = FileUtilities.ItemSpecModifiers.GetItemSpecModifier(null, _escapedItemSpec, _escapedDefiningProject, metadataName, ref _fullPath);
                }
                else if (_customEscapedMetadata != null)
                {
                    _customEscapedMetadata.TryGetValue(metadataName, out metadataValue);
                }

                return metadataValue ?? String.Empty;
            }

[Forgind]

I'm confused—what does this code have to do with this PR?

[benvillalobos]

LGTM

[benvillalobos]

I wasn't sure how it would ultimately was used, so I looked into it. Looks like it null checks anyway. LGTM

            /// <summary>
            /// Returns the escaped value of the requested metadata name.
            /// </summary>
            string ITaskItem2.GetMetadataValueEscaped(string metadataName)
            {
                ErrorUtilities.VerifyThrowArgumentNull(metadataName, nameof(metadataName));

                string metadataValue = null;

                if (FileUtilities.ItemSpecModifiers.IsDerivableItemSpecModifier(metadataName))
                {
                    // FileUtilities.GetItemSpecModifier is expecting escaped data, which we assume we already are.
                    // Passing in a null for currentDirectory indicates we are already in the correct current directory
                    metadataValue = FileUtilities.ItemSpecModifiers.GetItemSpecModifier(null, _escapedItemSpec, _escapedDefiningProject, metadataName, ref _fullPath);
                }
                else if (_customEscapedMetadata != null)
                {
                    _customEscapedMetadata.TryGetValue(metadataName, out metadataValue);
                }

                return metadataValue ?? String.Empty;
            }

[sfoslund]

Thanks @Forgind @benvillalobos! I have a SDK PR depending on this that I'm hoping to get in by the end of the week so if you can merge this whenever your builds are working that would be great, thanks!

[cdmihai]

Worth adding a test for this, that would have caught the exception?

[sfoslund]

Worth adding a test for this, that would have caught the exception?

I don't have a deep understanding of RAR so I'm not sure how I would write a test for this within your infrastructure. However, the issue is repro'ed in several SDK tests now that we are enabling OutputUnresolvedAssemblyConflicts, so there is test coverage downstream.

If anyone wants to take a deeper look into how this happens, I've provided Nathan a binlog with the error.

[cdmihai]

However, the issue is repro'ed in several SDK tests now that we are enabling OutputUnresolvedAssemblyConflicts, so there is test coverage downstream.

Since RAR is a pain to think about, downstream coverage sounds good enough to me :)