Fix rebuild round trip in unit tests#51877
Conversation
There was a problem hiding this comment.
shouldn't need to pass a logger [](start = 17, length = 31)
Could we implement a simple ILogger that drops all log statements and use that here if logger == null?
There was a problem hiding this comment.
We could but the goal is to just delete this field from the type entirely. I came close to making that change in this PR but decided to hold back and save it for another.
This changes the rebuild unit tests to fully round trip the `Compilation` they are building and verify the output byte for byte matches the input. Issue dotnet#51873 discovered working on the change
The parse options weren't correctly being hooked up to the `CompilationOptions` and that was breaking round tripping closes dotnet#51873
| cancellationToken); | ||
| } | ||
|
|
||
| public static unsafe EmitResult Emit( |
There was a problem hiding this comment.
Should we make this overload 'internal' or perhaps even 'private'?
| bool Debug, | ||
| string DebugPath); | ||
|
|
||
| internal record ResolvedSource( |
There was a problem hiding this comment.
I think I'm getting a little lost in the usage of these records:
- ResolvedSource
- SourceFileInfo
- SyntaxTreeInfo
Could you please clarify why it's necessary to have all 3?
There was a problem hiding this comment.
At the BuildConstructor level there is a need to essentially to pass down a tuple of (string FilePath, SourceText Text) to the language specific routine so that it can generate a SyntaxTree.
Before my change ResolvedSource served that purpose because we were only driving this operation from the BuildValidator tool. The ResolvedSource type is used for resolving source locations on disk, it contains the information necessary for creating SyntaxTree in BuildConstructor and hence it was used.
From the library / unit test layer though the ResolvedSource type contains a lot of unnecessary information for generating SyntaxTree. The caller needs to think about items like embedding, hashes, etc ... None of which is actually necessary to generate the SyntaxTree. I toyed for a bit with a pattern of having a method called CreateSyntaxTree(string filePath, SourceText sourceText) on BuildConstructor but that didn't feel right at this time. May be right later though when we get further into the abstractions.
Hence I decided to simplify and introduce SyntaxTreeInfo for the moment. I think it makes it clearer what the actual dependencies are for this layer.
As for why we need ResolvedSource and SourceFileInfo it's because one specifies what to retrieve and the other what was retrieved. It's possible we could get to a point where we don't need one of them but at the moment there didn't seem to be a clear way.
|
@RikkiGibson, @cston any other feedback I need to take care of here? |
This changes the rebuild unit tests to fully round trip the
Compilationthey are building and verify the output byte for bytematches the input.
Issue #51873 discovered working on the change
Note: this PR builds off of the changes in #51852 so make sure to review the last commit only here.