-
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
Rebuild compilations that use top-level statements #51845
Conversation
using System.IO; | ||
using System.Threading; | ||
using Microsoft.CodeAnalysis.VisualBasic; | ||
using System.Text; |
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.
Are all of these usings needed? #Closed
return new ResolvedSource(OnDiskPath: null, text, new SourceFileInfo(path, text.ChecksumAlgorithm, text.GetChecksum().ToArray(), text, embeddedCompressedHash: null)); | ||
}).ToImmutableArray(); | ||
var references = original.References.ToImmutableArray(); | ||
var (compilation, isError) = bc.CreateCompilation(optionsReader, path, sources, references); |
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.
Was this same pattern used in another test change? If so, could this be extracted to a helper method?
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 will add helpers in the near future after we get byte-for-byte equality working in unit tests.
The rebuild failures repro locally. Will have to figure out why I'm off by 1 byte in some cases after this change.
|
I found that the change to pass Compilation.GetEntryPoint() as the debugEntryPoint caused specific rebuilds to fail where the project has a Once we get the ability to do byte-for-byte equality in rebuild tests, we should add tests for various entry points, including async and Task-returning Main. |
: null; | ||
|
||
private (string MainTypeName, string MainMethodName)? GetMainMethodInfo() | ||
public (string MainTypeName, string MainMethodName)? GetMainMethodInfo() |
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.
This is a public
method on a public
type and hence tuples are generally discourage. Don't have to fix in this PR but feel like this will become a record
before we can "ship" this library.
: null; | ||
// Here we only want to give the caller the main type name if the main method is named "Main" per convention. | ||
// If the main method has another name, we have to assume that specifying a main type name won't work. | ||
// For example, if the compilation uses top-level statements. |
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.
Any reason you didn't make this an XML doc?
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 moved this to an inline comment for now. I am fine with it being either inline or XML doc, but it seems like it would be more appropriate on the "root" GetMethodMainInfo
if it were moved to XML doc.
It's not currently clear to me that CompilationOptionsReader is going to be part of the public API in the long term. For example, perhaps we will go back to accepting the PEReader+MetadataReader combo directly out of the feeling that we don't need to provide the individual facilities of the CompilationOptionsReader to library consumers.
@@ -155,6 +152,11 @@ public ImmutableArray<MetadataReferenceInfo> GetMetadataReferences() | |||
var mdReader = PeReader.GetMetadataReader(); | |||
var methodDefinition = mdReader.GetMethodDefinition(header.EntryPoint); | |||
var methodName = mdReader.GetString(methodDefinition.Name); | |||
if (methodName != WellKnownMemberNames.EntryPointMethodName) |
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.
Why return null
here vs. adding an extra field to the tuple that says IsTopLevelStatements
?
The current approach is in some ways hiding information and it's asking callers to infer behavior based on that information missing. Callers have to know that when this is null
then "ah it's top level statements, do another trick". Rather than that why not return all the info and explicitly state this is top level statements case?
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.
In my view, the purpose of the method is to answer the question "what main type name should I pass to the compilation/emit options." If the answer is "none", then it's ok to return null. Would be happy to rename this to try and make that more explicit.
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.
Here is another way of phrasing my question: today null
can represent two different outcomes
- Inability to read the metadata correctly
- Metadata read correctly but this is a program compiled with top level statements
Should consumers react the same way to both situations?
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.
- Inability to read the metadata correctly
I think if the tool can detect that the metadata is malformed, it might be good to throw and turn this into a misc error. But if the metadata is just absent, it feels ok for the consumer to do what they were doing before this PR--which is to react to the null by saying "ok, I just won't pass anything for the mainType or the debugEntryPoint."
|
||
namespace Microsoft.CodeAnalysis.Rebuild.UnitTests | ||
{ | ||
public class CSharpRebuildTests : CSharpTestBase |
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.
Why the new class here vs. adding onto the existing test case?
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 felt like "round tripping various compilation options" was pretty well delineated as a category of tests from "using various C# language features"
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.
What type of test do you imagine won't involve a round trip at the end though? That in many ways is the ultimate test of the code here 😄
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 all will test that round tripping works. But some tests are primarily about passing "unusual" compilation options while other tests are primarily about using "unusual" language features.
That said, if you want me to put this in the existing file, please let me know and I will.
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'll let you decide.
@@ -155,6 +152,11 @@ public ImmutableArray<MetadataReferenceInfo> GetMetadataReferences() | |||
var mdReader = PeReader.GetMetadataReader(); | |||
var methodDefinition = mdReader.GetMethodDefinition(header.EntryPoint); | |||
var methodName = mdReader.GetString(methodDefinition.Name); | |||
if (methodName != WellKnownMemberNames.EntryPointMethodName) |
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.
Here is another way of phrasing my question: today null
can represent two different outcomes
- Inability to read the metadata correctly
- Metadata read correctly but this is a program compiled with top level statements
Should consumers react the same way to both situations?
|
||
namespace Microsoft.CodeAnalysis.Rebuild.UnitTests | ||
{ | ||
public class CSharpRebuildTests : CSharpTestBase |
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'll let you decide.
Closes #51802
Addresses the following error