-
Notifications
You must be signed in to change notification settings - Fork 420
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
Correctly handle multiple reference aliases #1905
Conversation
@@ -671,7 +671,14 @@ private void UpdateProjectReferences(Project project, ImmutableArray<string> pro | |||
{ | |||
if (!string.IsNullOrEmpty(projectReferenceAliases)) | |||
{ | |||
aliases = projectReferenceAliases.Split(';').ToImmutableArray(); | |||
var trimmed = projectReferenceAliases.Split(','); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if both ,
and ;
are supported here, I'm not able to find much documentation however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are not - in the case that the element contains a semicolon-separated list of aliases, the build process will fail:
Roslyn\Microsoft.CSharp.Core.targets(70,5): error MSB6001: Invalid command line switch for "csc.dll". System.ArgumentException: MSB3053: The assembly alias "abc;def" on reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe just write it in one line
!string.IsNullOrWhiteSpace(projectReferenceAliases)
? ImmutableArray.CreateRange(projectReferenceAliases.Split(new char[] { ',' }, StringSplitOptions.RemoveEmptyEntries).Select(a => a.Trim()))
: ImmutableArray<string>.Empty;
this is exactly what Roslyn uses internally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done - I erred on the side of trying to avoid LINQ for performance, but I do suppose this isn't directly in the hotpath.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@david-driscoll what do you think? |
Seems good to me! |
The element of a or supports multiple aliases. Currently, OmniSharp will flag attempted usings of these aliases as errors, despite the fact that they are valid and the project will still build without issue.
I can find no hard documentation of this specification, but have manually tested to confirm the three primary points of this PR: