Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Rebuild compilations that use top-level statements #51845

Merged
merged 6 commits into from
Mar 17, 2021
Merged

Rebuild compilations that use top-level statements #51845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e093816
Rebuild compilations that use top-level statements
RikkiGibson Mar 12, 2021
c44eb97
Fix rebuilding projects with Task Main()
RikkiGibson Mar 15, 2021
a58b666
Remove unused usings
RikkiGibson Mar 15, 2021
26e7d3e
Update CompilationOptionsReader.cs
RikkiGibson Mar 16, 2021
8ab23fe
Merge branch 'main' of github.com:dotnet/roslyn into rebuild-entrypoint
RikkiGibson Mar 16, 2021
601e582
Fix test
RikkiGibson Mar 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion eng/test-rebuild.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,6 @@ try {
" --exclude netcoreapp3.1\VBSyntaxGenerator.dll" +

# Compilation Errors
" --exclude net5.0\IOperationGenerator.dll" +
" --exclude net472\Microsoft.CodeAnalysis.CSharp.Scripting.Desktop.UnitTests.dll" +
" --exclude netcoreapp3.1\Microsoft.CodeAnalysis.CSharp.Scripting.dll" +
" --exclude netstandard2.0\Microsoft.CodeAnalysis.CSharp.Scripting.dll" +
Expand Down
3 changes: 1 addition & 2 deletions src/Compilers/Core/Rebuild/BuildConstructor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,8 +323,7 @@ public static unsafe EmitResult Emit(

IMethodSymbol? getDebugEntryPoint()
{
if (optionsReader.GetMainTypeName() is { } mainTypeName &&
optionsReader.GetMainMethodName() is { } mainMethodName)
if (optionsReader.GetMainMethodInfo() is (string mainTypeName, string mainMethodName))
{
var typeSymbol = producedCompilation.GetTypeByMetadataName(mainTypeName);
if (typeSymbol is object)
Expand Down
19 changes: 11 additions & 8 deletions src/Compilers/Core/Rebuild/CompilationOptionsReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,15 +139,9 @@ public OutputKind GetOutputKind() =>
? OutputKind.ConsoleApplication
: OutputKind.DynamicallyLinkedLibrary;

public string? GetMainTypeName() => GetMainMethodInfo() is { } tuple
? tuple.MainTypeName
: null;
public string? GetMainTypeName() => GetMainMethodInfo()?.MainTypeName;

public string? GetMainMethodName() => GetMainMethodInfo() is { } tuple
? tuple.MainMethodName
: null;

private (string MainTypeName, string MainMethodName)? GetMainMethodInfo()
public (string MainTypeName, string MainMethodName)? GetMainMethodInfo()
Copy link
Member

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.

{
if (!(PdbReader.DebugMetadataHeader is { } header) ||
header.EntryPoint.IsNil)
Expand All @@ -158,6 +152,15 @@ public OutputKind GetOutputKind() =>
var mdReader = PeReader.GetMetadataReader();
var methodDefinition = mdReader.GetMethodDefinition(header.EntryPoint);
var methodName = mdReader.GetString(methodDefinition.Name);

// Here we only want to give the caller the main method name and containing type name if the 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.
if (methodName != WellKnownMemberNames.EntryPointMethodName)
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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

  1. Inability to read the metadata correctly
  2. Metadata read correctly but this is a program compiled with top level statements

Should consumers react the same way to both situations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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."

{
return null;
}

var typeHandle = methodDefinition.GetDeclaringType();
var typeDefinition = mdReader.GetTypeDefinition(typeHandle);
var typeName = mdReader.GetString(typeDefinition.Name);
Expand Down
51 changes: 51 additions & 0 deletions src/Compilers/Core/RebuildTest/CSharpRebuildTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Immutable;
using System.Linq;
using System.Reflection.PortableExecutable;
using BuildValidator;
using Microsoft.CodeAnalysis.CSharp.Test.Utilities;
using Microsoft.CodeAnalysis.Emit;
using Microsoft.CodeAnalysis.Test.Utilities;
using Microsoft.Extensions.Logging;
using Xunit;

namespace Microsoft.CodeAnalysis.Rebuild.UnitTests
{
public class CSharpRebuildTests : CSharpTestBase
Copy link
Member

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?

Copy link
Contributor Author

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"

Copy link
Member

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 😄

Copy link
Contributor Author

@RikkiGibson RikkiGibson Mar 16, 2021 •
edited
Loading

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.

Copy link
Member

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.

{
[Fact]
public void TopLevelStatements()
{
const string path = "test";

var original = CreateCompilation(
@"System.Console.WriteLine(""I'm using top-level statements!"");",
options: TestOptions.DebugExe);

original.VerifyDiagnostics();

var originalBytes = original.EmitToArray(new EmitOptions(debugInformationFormat: DebugInformationFormat.Embedded));
var peReader = new PEReader(originalBytes);
Assert.True(peReader.TryOpenAssociatedPortablePdb(path, path => null, out var provider, out _));
var pdbReader = provider!.GetMetadataReader();

var factory = LoggerFactory.Create(configure => { });
var logger = factory.CreateLogger(path);
var bc = new BuildConstructor(logger);

var optionsReader = new CompilationOptionsReader(logger, pdbReader, peReader);

var sources = original.SyntaxTrees.Select(st =>
{
var text = st.GetText();
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 = bc.CreateCompilation(optionsReader, path, sources, references);
compilation.VerifyEmitDiagnostics();
}
}
}