Fix #line directive in rebuild scenarios#52960
Fix #line directive in rebuild scenarios#52960jaredpar merged 17 commits intodotnet:release/dev16.11from
Conversation
This adds proper support for handling `#line` and `#ExternalSource` directives during rebuild scenarios. There are actually two different problems that need to be solved here. The first is handling of the paths in the `NormalizePath` operation. The names used in these directives can be relative paths. That means during initial compilation they are normalized in the context of the containing file name. That always succeeds because in initial compilation the containing file name is a legal file path. During rebuild though the containing file name may not be a legal path; it could be running a rebuild where `/pathmap` was used to create non-legal paths on the current operating system. Hence if nothing is done then normalization will return the literal name in the source file. That causes conflicts because the same name can be used in different files in different directories. Hence we need to provide a `SourceReferenceResolver` that gives unique paths for every `#line` pairing. The second is that we need to add back in the non-source file documents into the PDB after the source files are written. That requires extra data being passed down to the emit phase
|
@dotnet/roslyn-compiler PTAL |
|
|
||
| var compilationState = new TypeCompilationState(null, _compilation, _moduleBeingBuiltOpt); | ||
| foreach (Cci.IMethodDefinition definition in privateImplClass.GetMethods(new EmitContext(_moduleBeingBuiltOpt, null, diagnostics.DiagnosticBag, metadataOnly: false, includePrivateMembers: true))) | ||
| foreach (Cci.IMethodDefinition definition in privateImplClass.GetMethods(new EmitContext(_moduleBeingBuiltOpt, null, null, diagnostics.DiagnosticBag, metadataOnly: false, includePrivateMembers: true))) |
There was a problem hiding this comment.
nit: it would be nice to label these null literals the same way we label the bool literals
There was a problem hiding this comment.
Agree. I wanted to make sure we all agreed that EmitContext was the right way to flow data here (put acomment in the PR to get clarity on that). If we all agree I'll go back and clean this up. Didn't want to do the work if we were going to decide to thread it through separately cause then I'd have to also undo it.
|
The non-windows test failures look real. I'm digging into them. Strangely they are VB only. |
|
Will take a look next week. |
|
Sorry I keep pushing commits that say "fix tests" only to have the tests break again. I'm in a situation where the tests pass on one OS but fail on the other. So I fix everything on Windows, push, see Linux break, fix on Linux, push, watch Windows break, rinse repeat. It's a bit frustrating but think I'm at the bottom now. |
| var name = document.Location; | ||
| if (_sourceDebugDocumentsAdded && this.Context.RebuildDataOpt is { } rebuildData) | ||
| { | ||
| name = rebuildData.GetNextNonSourceFileDocumentName(); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Unfortunately no we can't.
Have to get into how #line are processed a bit to understand the problem here. The #line directives are processed initially in GenerateMethodBody and that runs in parallel across the compilation. As they are processed there we essentially have to do two operations:
- Generate a path via
NormalizePath. The invariant here is that for two calls toNormalizePathfor the same logical file name in a#linedirective we need to return the same string. By "logical name" I mean that two files in the same directive with a#line 42 "data.txt"need to return the same name but a file in a different directory with the same#line 42 "data.txt"needs to return a different name. - Generate a
DebugSourceDocumentfor that path. As long as we maintain the invariant above this will just work out.
This gets us up to the point where we set _sourceDebugDocumentsAdded to true. At this point we have:
- The dictionary of
string(the path) to theDebugSourceDocumententries - The set of non-source file names we need to emit into the PDB
While we absolutely know the order in which (2) needs to be emitted we don't know the order of (1) and that's important. The final stage of emit is going to lookup DebugSourceDocument in the same order as the original compilation via AddDocument and we have to make sure all the identities match here (particularly the numeric token attached with the names, documents, etc ...) It's also not possible to map (2) back to (1) without a decent investment in how we deal with full paths.
In chatting with @tmat we favored just hooking AddDocument since because of determinism we understood that call will come in a predictable order.
I think with enough work I could make it possible to do the name mapping correctly in NormalizePath but I worry I'm going to miss alot of casese. Particularly if users invest in very non-standard pathing conventions in /pathmap.
Very open to suggestions here if you see a better way.
| return @$"{root}\{path}"; | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
It can't be return @$"{baseFilePath}\{path}"; because we need #line directives in different files in the same directory to normalize to the same final path. If we took this suggestion then we would end up with two documents instead of one.
|
Responded to PR feedback |
| RebuildData? rebuildDataOpt = null, | ||
| DiagnosticBag? diagnostics = null, | ||
| bool metadataOnly = false, | ||
| bool includePrivateMembers = true) |
There was a problem hiding this comment.
Are we relying on any of these default values currently? If not, consider removing the default values since it feels like callers should be explicit about some of these values, particularly diagnostics which should perhaps be non-nullable.
There was a problem hiding this comment.
Took this approach to minimize the diff for now. Wanted to wait on @tmat to give feedback on whether this was an appropriate way to pass around RebuildData. If he confirms I'll remove all the default values here and clean up the callers.
|
It looks like there is a formatting violation and some test failures. |
| { | ||
| get | ||
| { | ||
| // WSL environments can have this value as empty string |
There was a problem hiding this comment.
were you missing some english-only tests on linux locally?
There was a problem hiding this comment.
Yes. Several of the VB rebuild tests were english only and I couldn't run them on my WSL environment but they were failing in CI. This unblocked me.
| public bool IsRefAssembly => MetadataOnly && !IncludePrivateMembers; | ||
|
|
||
| public EmitContext(CommonPEModuleBuilder module, SyntaxNode syntaxNodeOpt, DiagnosticBag diagnostics, bool metadataOnly, bool includePrivateMembers) | ||
| public EmitContext(CommonPEModuleBuilder module, SyntaxNode? syntaxNodeOpt, DiagnosticBag diagnostics, bool metadataOnly, bool includePrivateMembers) |
There was a problem hiding this comment.
| public EmitContext(CommonPEModuleBuilder module, SyntaxNode? syntaxNodeOpt, DiagnosticBag diagnostics, bool metadataOnly, bool includePrivateMembers) | |
| public EmitContext(CommonPEModuleBuilder module, SyntaxNode? syntaxNodeOpt, DiagnosticBag diagnostics, bool metadataOnly, bool includePrivateMembers) |
Co-authored-by: Rikki Gibson <rikkigibson@gmail.com>
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
This adds proper support for handling
#lineand#ExternalSourcedirectives during rebuild scenarios. There are actually two
different problems that need to be solved here.
The first is handling of the paths in the
NormalizePathoperation.The names used in these directives can be relative paths. That means
during initial compilation they are normalized in the context of the
containing file name. That always succeeds because in initial
compilation the containing file name is a legal file path.
During rebuild though the containing file name may not be a legal
path; it could be running a rebuild where
/pathmapwas used to createnon-legal paths on the current operating system. Hence if nothing is
done then normalization will return the literal name in the source file.
That causes conflicts because the same name can be used in different
files in different directories. Hence we need to provide a
SourceReferenceResolverthat gives unique paths for every#linepairing.
The second is that we need to add back in the non-source file documents
into the PDB after the source files are written. That requires extra
data being passed down to the emit phase