Rationalize how temporary directory works today

[jaredpar]

Before fixing #65415 I needed to better rationalize how
Path.GetTempPath and temp paths in general worked in the compiler.
This change ended up addressing the following problems:

Path.GetTempPath

Path.GetTempPath should never be used in the compiler libraries.
That uses a number of environment variables, like working directory,
that can change during the lifetime of a build operation. That makes
compilation non-deterministic and can lead to sporadic errors. All
uses of GetTempPath need to come from the initial csc / vbc
invocation and be passed through as arguments.

This was solved by adding a new BannedSymbols file for compiler
libraries that banned Path.GetTempPath and working through the
remaining case I found.

This introduced one new error because Path.GetTempPath was being used
as a fall back in legacy signing if csc / vbc couldn't calculate a
temp path at invocation time. This is a "this should never actually
happen" problem but I added an error code as a precaution.

I did not force the temp directory to non-null for all compilations
because the vast majority of compilations don't actually need a temp
directory. It is only used in legacy signing which is very rare.

Where is temp required

It was unclear in our code base where a temp directory was actually
required for execution. To get a better handle on this, now and going
forward, I enabled NRT in files that handled temp directories. After
working through it there are only two cases where it's required:

1. When using legacy strong named signing.
2. When starting the compiler server.

For (2) it's required as a place to shadow copy analyzers /
generators. There is no reasonable fall back if the server can't
calculate a temp path. Further if the client can't calculate a temp path
then there is a very strong chance (basically guaranteed) that the
server can't either. Therefore I added a few places where we simply
don't even try to communicate with the compiler server if we can't find
a temporary directory. Basically fail fast vs. failing slow.

Once this change is in I can move forward with #65415. The impact of
that change will be much clearer after this goes through because it
changes both what we depend on and where shadow copy happens.

[jaredpar]

@dotnet/roslyn-compiler PTAL

[RikkiGibson]

Since we don't want analyzers/generators doing I/O generally, it follows that we don't want them using temp directories for scratch space, etc., right?

[jaredpar]

Since we don't want analyzers/generators doing I/O generally, it follows that we don't want them using temp directories for scratch space, etc., right?

Most definitely. If that's not o nthe banned list yet it needs to be.

[cston]

        private Stream? _streamToDispose;

This field is now duplicated in the base class. As an alternative, what if we remove the field from the base class and remove CreateStream() and change GetOrCreateStream() to the abstract method that is overridden in the two derived classes?

---

Refers to: src/Compilers/Core/Portable/CommandLine/CommonCompiler.CompilerEmitStreamProvider.cs:24 in eca53ed. [](commit_id = eca53ed, deletion_comment = False)

[RikkiGibson]

Is this related to the PR?

[jaredpar]

Yes. The virtual member was removed in the base type.

The reason for this part of the change is I had a lot of time understanding the relationship between the EmitStreamProvider and underlying Stream. Particularly when do they get created and what is the ownership model.
A significant part of this change was figuring out exactly where the files were created and destroyed hence as part of the fix I tried to make this much clearer.

Happy for feedback on the new approach.

[RikkiGibson]

Is this deletion related to the goal of the PR?

[jaredpar]

@RikkiGibson FYI i pushed on more commit that had functional impact. In reviewing my change this morning and thinking about the followup change I realized I'd error'd in one place. Specifically not sending compilation requests to the server when the request cannot calculate a temporary directory. Originally I did that under the logic of the server can't start if there is a missing temp directory so what's the point. I now realize that was wrong for two reasons:

1. The temp directory calculated by the client and server can, and sometimes will be very different. Love or hate it builds change environment variables and that can change what value we get for a temp. Just because the client can't calculate a temp doesn't mean the server can't either. It could have been running from a previous request where environment variables were differente.
2. A build request does not actually need a temporary directory in the 99.9% case. So treating them as invalid is wrong.
3. The responsibility was just in the wrong place. The server is the best to know if it can / can't startup. There is already loads of logic to deal with failed start up. The client should just send the request and let the server deal with startup conditions.

Once I start on the next change I'm goingto add the fast fail code for the missing temp dir case. It's an extreme edge anyways.

[RikkiGibson]

Got it, so you just reverted the check to ensure client can get a temp directory before sending the build request? And in a subsequent PR you'll add some new check to ensure the server fails in a clear/actionable way when it can't create a temp directory?