-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Remove use of Environment.CurrentDirectory #69564
Conversation
The compiler should not make use of `Environment.CurrentDirectory` in the libraries layer. That layer cannot depend on anything environment related because doing so would break cases like VBCSCompiler which needs to be environment agnostic. The one use was in `AdditionalSourcesCollection` to verify that the provided hint name was within the current working directory. After some thought I decided the best course was to remove this restriction. The ratoinale is once you actually thread through the current directory you find that it must be added to the `Compilation`. Effectively everything that hosts a `Compilation` must provide a working directory and that goes against the goals of `Compilation` which seeks to be agnostic to working directories. Think allowing customers to specify the full paths was the lesser evil here. closes dotnet#69516
@dotnet/roslyn-compiler PTAL |
So I believe the original intention behind this was to stop people from essentially doing We probably don't need that, if a generator wants to be malicious it can just I think its probably fine to just remove it, or we could potentially add |
@@ -84,11 +83,6 @@ public void Add(string hintName, SourceText source) | |||
throw new ArgumentException(string.Format(CodeAnalysisResources.SourceTextRequiresEncoding, hintName), nameof(source)); | |||
} | |||
|
|||
if (Path.IsPathRooted(hintName) || !Path.GetFullPath(hintName).StartsWith(Environment.CurrentDirectory, _hintNameComparison)) |
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.
We could potentially keep the Path.IsPathRooted
check if we think its worth it?
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.
If we keep it though and fire and error then we potentially break existing code. 😦
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 Thanks (iteration 1)
The compiler should not make use of
Environment.CurrentDirectory
in the libraries layer. That layer cannot depend on anything environment related because doing so would break cases like VBCSCompiler which needs to be environment agnostic.The one use was in
AdditionalSourcesCollection
to verify that the provided hint name was within the current working directory. After some thought I decided the best course was to remove this restriction. The ratoinale is once you actually thread through the current directory you find that it must be added to theCompilation
. Effectively everything that hosts aCompilation
must provide a working directory and that goes against the goals ofCompilation
which seeks to be agnostic to working directories.Think allowing customers to specify the full paths was the lesser evil here.
closes #69516