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.

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

Merged
merged 6 commits into from
Mar 17, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
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
22 changes: 1 addition & 21 deletions src/Compilers/Core/Rebuild/BuildConstructor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -298,8 +298,6 @@ public static unsafe EmitResult Emit(
.Select(pair => EmbeddedText.FromSource(pair.path, pair.text))
.ToImmutableArray();

var debugEntryPoint = getDebugEntryPoint();

var emitResult = producedCompilation.Emit(
peStream: rebuildPeStream,
pdbStream: null,
Expand All @@ -311,32 +309,14 @@ public static unsafe EmitResult Emit(
debugInformationFormat: DebugInformationFormat.Embedded,
highEntropyVirtualAddressSpace: (peHeader.DllCharacteristics & DllCharacteristics.HighEntropyVirtualAddressSpace) != 0,
subsystemVersion: SubsystemVersion.Create(peHeader.MajorSubsystemVersion, peHeader.MinorSubsystemVersion)),
debugEntryPoint: debugEntryPoint,
debugEntryPoint: producedCompilation.GetEntryPoint(cancellationToken),
metadataPEStream: null,
pdbOptionsBlobReader: optionsReader.GetMetadataCompilationOptionsBlobReader(),
sourceLinkStream: sourceLink != null ? new MemoryStream(sourceLink) : null,
embeddedTexts: embeddedTexts,
cancellationToken: cancellationToken);

return emitResult;

IMethodSymbol? getDebugEntryPoint()
{
if (optionsReader.GetMainTypeName() is { } mainTypeName &&
optionsReader.GetMainMethodName() is { } mainMethodName)
{
var typeSymbol = producedCompilation.GetTypeByMetadataName(mainTypeName);
if (typeSymbol is object)
{
var methodSymbols = typeSymbol
.GetMembers(mainMethodName)
.OfType<IMethodSymbol>();
return methodSymbols.FirstOrDefault();
}
}

return null;
}
}
}
}
11 changes: 5 additions & 6 deletions src/Compilers/Core/Rebuild/CompilationOptionsReader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -136,12 +136,11 @@ public OutputKind GetOutputKind() =>
? OutputKind.ConsoleApplication
: OutputKind.DynamicallyLinkedLibrary;

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

public string? GetMainMethodName() => GetMainMethodInfo() is { } tuple
? tuple.MainMethodName
// 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.
Copy link
Member

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?

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

public string? GetMainTypeName() => GetMainMethodInfo() is (string typeName, WellKnownMemberNames.EntryPointMethodName)
? typeName
: null;

private (string MainTypeName, string MainMethodName)? GetMainMethodInfo()
Expand Down
62 changes: 62 additions & 0 deletions src/Compilers/Core/RebuildTest/CSharpRebuildTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// 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.Generic;
using System.Collections.Immutable;
using System.Linq;
using System;
using System.Reflection.PortableExecutable;
using BuildValidator;
using Castle.Core.Logging;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Test.Utilities;
using Microsoft.CodeAnalysis.Emit;
using Microsoft.CodeAnalysis.Test.Utilities;
using Microsoft.Extensions.Logging;
using Xunit;
using System.IO;
using System.Threading;
using Microsoft.CodeAnalysis.VisualBasic;
using System.Text;
Copy link
Member

@cston cston Mar 12, 2021

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


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

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, isError) = bc.CreateCompilation(optionsReader, path, sources, references);
Copy link
Member

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?

Copy link
Contributor Author

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.


Assert.False(isError);
compilation.VerifyEmitDiagnostics();
}
}
}