Enable managed ilasm round trip testing.

[jkoritzinsky]

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

Note

This PR description was generated with GitHub Copilot.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[jkoritzinsky]

/azp run runtime-coreclr ilasm

[azure-pipelines]

Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

This PR updates the runtime test pipeline template to run an additional ILASM round-trip scenario that uses the managed ILASM implementation when the ilasm test group is selected.

Changes:

• Adds managedilasmroundtrip to the ilasm test group’s Helix scenarios list (alongside ilasmroundtrip).

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 25 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/tools/ilasm/tests/ILAssembler.Tests/AssemblyTests.cs:349

• ManagedIlasm_DeterministicOutput_IsByteIdentical varies the output file extension between .dll and .exe, but the Options passed to the compiler never sets Dll. That means the ".dll" case still produces an image whose COFF Characteristics might not have the DLL bit set, so the test doesn't actually exercise the new Options.Dll behavior and can mask regressions in DLL-vs-EXE emission.

 var options = new Options
{
Deterministic = true,
OutputFileName = outputFileName,
};

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• DocumentCompiler.Compile changes its public return type from PEBuilder? to CompilationResult?. Since DocumentCompiler is public, this is a breaking change for any external consumers of ILAssembler, and it also introduces/expands public surface area (CompilationResult). If ILAssembler isn't intended to be a supported public API, consider making the types/members internal instead; otherwise, consider keeping the existing API shape (e.g., retaining a PEBuilder?-returning entry point) to avoid breaking downstream code.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[Copilot]

Pull request overview

Copilot reviewed 22 out of 26 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/tools/ilasm/tests/ILAssembler.Tests/AssemblyTests.cs:410

• The deterministic-output test varies the output file name between .dll and .exe, but the Options passed to the compiler never sets Options.Dll. This means the “Deterministic.dll” case still produces an executable image, so the test isn’t exercising the DLL imageCharacteristics path (and could miss DLL-specific nondeterminism).

 var options = new Options
{
Deterministic = true,
OutputFileName = outputFileName,
};

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• This changes the public API of ILAssembler by replacing the public Compile return type (PEBuilder?) with a new public CompilationResult and updating the tuple signatures. The PR description doesn’t link an api-approved issue, so this can’t be validated against an approved API shape per the repo’s API approval process. Either (a) link an approved issue, or (b) keep the public surface unchanged and move the new behavior behind internal APIs / internal helper types.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[Copilot]

Pull request overview

Copilot reviewed 25 out of 29 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tools/ilasm/tests/ILAssembler.Tests/GenericTests.cs:167

• GenericType_MoreThanMetadataIndexRange_IsAcceptedForCompatibility builds a very large IL source string (65,537 generic parameters). Initializing StringBuilder without an estimated capacity will cause repeated growth reallocations and extra copying, which can significantly slow this test and increase memory usage in CI.

Consider supplying an initial capacity that scales with GenericParameterCount to keep this test cheaper while preserving coverage.

 StringBuilder source = new("""
.assembly extern mscorlib { }
.assembly test { }
.class public auto ansi Test<
""");

[Copilot]

Pull request overview

Copilot reviewed 26 out of 30 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• DocumentCompiler is public, and this change replaces its public Compile(...) return type from PEBuilder? to CompilationResult? (and adds a new public type). That’s a source-breaking public API change for any out-of-repo consumers of ILAssembler, and it also triggers the repo’s “new public API requires api-approved issue” process.

If ILAssembler isn’t intended to expose a stable public API, consider making DocumentCompiler (and/or the new CompilationResult) internal instead. If it is intended as public API, please link an api-approved issue in the PR description and ensure the API shape is approved before merging.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[jkoritzinsky]

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

Note

This PR description was generated with GitHub Copilot.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[jkoritzinsky]

/azp run runtime-coreclr ilasm

[azure-pipelines]

Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

This PR updates the runtime test pipeline template to run an additional ILASM round-trip scenario that uses the managed ILASM implementation when the ilasm test group is selected.

Changes:

• Adds managedilasmroundtrip to the ilasm test group’s Helix scenarios list (alongside ilasmroundtrip).

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 25 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/tools/ilasm/tests/ILAssembler.Tests/AssemblyTests.cs:349

• ManagedIlasm_DeterministicOutput_IsByteIdentical varies the output file extension between .dll and .exe, but the Options passed to the compiler never sets Dll. That means the ".dll" case still produces an image whose COFF Characteristics might not have the DLL bit set, so the test doesn't actually exercise the new Options.Dll behavior and can mask regressions in DLL-vs-EXE emission.

 var options = new Options
{
Deterministic = true,
OutputFileName = outputFileName,
};

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• DocumentCompiler.Compile changes its public return type from PEBuilder? to CompilationResult?. Since DocumentCompiler is public, this is a breaking change for any external consumers of ILAssembler, and it also introduces/expands public surface area (CompilationResult). If ILAssembler isn't intended to be a supported public API, consider making the types/members internal instead; otherwise, consider keeping the existing API shape (e.g., retaining a PEBuilder?-returning entry point) to avoid breaking downstream code.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[Copilot]

Pull request overview

Copilot reviewed 22 out of 26 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/tools/ilasm/tests/ILAssembler.Tests/AssemblyTests.cs:410

• The deterministic-output test varies the output file name between .dll and .exe, but the Options passed to the compiler never sets Options.Dll. This means the “Deterministic.dll” case still produces an executable image, so the test isn’t exercising the DLL imageCharacteristics path (and could miss DLL-specific nondeterminism).

 var options = new Options
{
Deterministic = true,
OutputFileName = outputFileName,
};

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• This changes the public API of ILAssembler by replacing the public Compile return type (PEBuilder?) with a new public CompilationResult and updating the tuple signatures. The PR description doesn’t link an api-approved issue, so this can’t be validated against an approved API shape per the repo’s API approval process. Either (a) link an approved issue, or (b) keep the public surface unchanged and move the new behavior behind internal APIs / internal helper types.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[Copilot]

Pull request overview

Copilot reviewed 25 out of 29 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tools/ilasm/tests/ILAssembler.Tests/GenericTests.cs:167

• GenericType_MoreThanMetadataIndexRange_IsAcceptedForCompatibility builds a very large IL source string (65,537 generic parameters). Initializing StringBuilder without an estimated capacity will cause repeated growth reallocations and extra copying, which can significantly slow this test and increase memory usage in CI.

Consider supplying an initial capacity that scales with GenericParameterCount to keep this test cheaper while preserving coverage.

 StringBuilder source = new("""
.assembly extern mscorlib { }
.assembly test { }
.class public auto ansi Test<
""");

[Copilot]

Pull request overview

Copilot reviewed 26 out of 30 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• DocumentCompiler is public, and this change replaces its public Compile(...) return type from PEBuilder? to CompilationResult? (and adds a new public type). That’s a source-breaking public API change for any out-of-repo consumers of ILAssembler, and it also triggers the repo’s “new public API requires api-approved issue” process.

If ILAssembler isn’t intended to expose a stable public API, consider making DocumentCompiler (and/or the new CompilationResult) internal instead. If it is intended as public API, please link an api-approved issue in the PR description and ensure the API shape is approved before merging.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[jkoritzinsky]

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

Note

This PR description was generated with GitHub Copilot.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[jkoritzinsky]

/azp run runtime-coreclr ilasm

[azure-pipelines]

Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

This PR updates the runtime test pipeline template to run an additional ILASM round-trip scenario that uses the managed ILASM implementation when the ilasm test group is selected.

Changes:

• Adds managedilasmroundtrip to the ilasm test group’s Helix scenarios list (alongside ilasmroundtrip).

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 25 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/tools/ilasm/tests/ILAssembler.Tests/AssemblyTests.cs:349

• ManagedIlasm_DeterministicOutput_IsByteIdentical varies the output file extension between .dll and .exe, but the Options passed to the compiler never sets Dll. That means the ".dll" case still produces an image whose COFF Characteristics might not have the DLL bit set, so the test doesn't actually exercise the new Options.Dll behavior and can mask regressions in DLL-vs-EXE emission.

 var options = new Options
{
Deterministic = true,
OutputFileName = outputFileName,
};

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• DocumentCompiler.Compile changes its public return type from PEBuilder? to CompilationResult?. Since DocumentCompiler is public, this is a breaking change for any external consumers of ILAssembler, and it also introduces/expands public surface area (CompilationResult). If ILAssembler isn't intended to be a supported public API, consider making the types/members internal instead; otherwise, consider keeping the existing API shape (e.g., retaining a PEBuilder?-returning entry point) to avoid breaking downstream code.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[Copilot]

Pull request overview

Copilot reviewed 22 out of 26 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/tools/ilasm/tests/ILAssembler.Tests/AssemblyTests.cs:410

• The deterministic-output test varies the output file name between .dll and .exe, but the Options passed to the compiler never sets Options.Dll. This means the “Deterministic.dll” case still produces an executable image, so the test isn’t exercising the DLL imageCharacteristics path (and could miss DLL-specific nondeterminism).

 var options = new Options
{
Deterministic = true,
OutputFileName = outputFileName,
};

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• This changes the public API of ILAssembler by replacing the public Compile return type (PEBuilder?) with a new public CompilationResult and updating the tuple signatures. The PR description doesn’t link an api-approved issue, so this can’t be validated against an approved API shape per the repo’s API approval process. Either (a) link an approved issue, or (b) keep the public surface unchanged and move the new behavior behind internal APIs / internal helper types.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[Copilot]

Pull request overview

Copilot reviewed 25 out of 29 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tools/ilasm/tests/ILAssembler.Tests/GenericTests.cs:167

• GenericType_MoreThanMetadataIndexRange_IsAcceptedForCompatibility builds a very large IL source string (65,537 generic parameters). Initializing StringBuilder without an estimated capacity will cause repeated growth reallocations and extra copying, which can significantly slow this test and increase memory usage in CI.

Consider supplying an initial capacity that scales with GenericParameterCount to keep this test cheaper while preserving coverage.

 StringBuilder source = new("""
.assembly extern mscorlib { }
.assembly test { }
.class public auto ansi Test<
""");

[Copilot]

Pull request overview

Copilot reviewed 26 out of 30 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• DocumentCompiler is public, and this change replaces its public Compile(...) return type from PEBuilder? to CompilationResult? (and adds a new public type). That’s a source-breaking public API change for any out-of-repo consumers of ILAssembler, and it also triggers the repo’s “new public API requires api-approved issue” process.

If ILAssembler isn’t intended to expose a stable public API, consider making DocumentCompiler (and/or the new CompilationResult) internal instead. If it is intended as public API, please link an api-approved issue in the PR description and ensure the API shape is approved before merging.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[jkoritzinsky]

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

Note

This PR description was generated with GitHub Copilot.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[jkoritzinsky]

/azp run runtime-coreclr ilasm

[azure-pipelines]

Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

This PR updates the runtime test pipeline template to run an additional ILASM round-trip scenario that uses the managed ILASM implementation when the ilasm test group is selected.

Changes:

• Adds managedilasmroundtrip to the ilasm test group’s Helix scenarios list (alongside ilasmroundtrip).

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 25 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/tools/ilasm/tests/ILAssembler.Tests/AssemblyTests.cs:349

• ManagedIlasm_DeterministicOutput_IsByteIdentical varies the output file extension between .dll and .exe, but the Options passed to the compiler never sets Dll. That means the ".dll" case still produces an image whose COFF Characteristics might not have the DLL bit set, so the test doesn't actually exercise the new Options.Dll behavior and can mask regressions in DLL-vs-EXE emission.

 var options = new Options
{
Deterministic = true,
OutputFileName = outputFileName,
};

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• DocumentCompiler.Compile changes its public return type from PEBuilder? to CompilationResult?. Since DocumentCompiler is public, this is a breaking change for any external consumers of ILAssembler, and it also introduces/expands public surface area (CompilationResult). If ILAssembler isn't intended to be a supported public API, consider making the types/members internal instead; otherwise, consider keeping the existing API shape (e.g., retaining a PEBuilder?-returning entry point) to avoid breaking downstream code.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[Copilot]

Pull request overview

Copilot reviewed 22 out of 26 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/tools/ilasm/tests/ILAssembler.Tests/AssemblyTests.cs:410

• The deterministic-output test varies the output file name between .dll and .exe, but the Options passed to the compiler never sets Options.Dll. This means the “Deterministic.dll” case still produces an executable image, so the test isn’t exercising the DLL imageCharacteristics path (and could miss DLL-specific nondeterminism).

 var options = new Options
{
Deterministic = true,
OutputFileName = outputFileName,
};

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• This changes the public API of ILAssembler by replacing the public Compile return type (PEBuilder?) with a new public CompilationResult and updating the tuple signatures. The PR description doesn’t link an api-approved issue, so this can’t be validated against an approved API shape per the repo’s API approval process. Either (a) link an approved issue, or (b) keep the public surface unchanged and move the new behavior behind internal APIs / internal helper types.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[Copilot]

Pull request overview

Copilot reviewed 25 out of 29 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tools/ilasm/tests/ILAssembler.Tests/GenericTests.cs:167

• GenericType_MoreThanMetadataIndexRange_IsAcceptedForCompatibility builds a very large IL source string (65,537 generic parameters). Initializing StringBuilder without an estimated capacity will cause repeated growth reallocations and extra copying, which can significantly slow this test and increase memory usage in CI.

Consider supplying an initial capacity that scales with GenericParameterCount to keep this test cheaper while preserving coverage.

 StringBuilder source = new("""
.assembly extern mscorlib { }
.assembly test { }
.class public auto ansi Test<
""");

[Copilot]

Pull request overview

Copilot reviewed 26 out of 30 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• DocumentCompiler is public, and this change replaces its public Compile(...) return type from PEBuilder? to CompilationResult? (and adds a new public type). That’s a source-breaking public API change for any out-of-repo consumers of ILAssembler, and it also triggers the repo’s “new public API requires api-approved issue” process.

If ILAssembler isn’t intended to expose a stable public API, consider making DocumentCompiler (and/or the new CompilationResult) internal instead. If it is intended as public API, please link an api-approved issue in the PR description and ensure the API shape is approved before merging.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[jkoritzinsky]

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

Note

This PR description was generated with GitHub Copilot.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[jkoritzinsky]

/azp run runtime-coreclr ilasm

[azure-pipelines]

Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

This PR updates the runtime test pipeline template to run an additional ILASM round-trip scenario that uses the managed ILASM implementation when the ilasm test group is selected.

Changes:

• Adds managedilasmroundtrip to the ilasm test group’s Helix scenarios list (alongside ilasmroundtrip).

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 25 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/tools/ilasm/tests/ILAssembler.Tests/AssemblyTests.cs:349

• ManagedIlasm_DeterministicOutput_IsByteIdentical varies the output file extension between .dll and .exe, but the Options passed to the compiler never sets Dll. That means the ".dll" case still produces an image whose COFF Characteristics might not have the DLL bit set, so the test doesn't actually exercise the new Options.Dll behavior and can mask regressions in DLL-vs-EXE emission.

 var options = new Options
{
Deterministic = true,
OutputFileName = outputFileName,
};

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• DocumentCompiler.Compile changes its public return type from PEBuilder? to CompilationResult?. Since DocumentCompiler is public, this is a breaking change for any external consumers of ILAssembler, and it also introduces/expands public surface area (CompilationResult). If ILAssembler isn't intended to be a supported public API, consider making the types/members internal instead; otherwise, consider keeping the existing API shape (e.g., retaining a PEBuilder?-returning entry point) to avoid breaking downstream code.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[Copilot]

Pull request overview

Copilot reviewed 22 out of 26 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/tools/ilasm/tests/ILAssembler.Tests/AssemblyTests.cs:410

• The deterministic-output test varies the output file name between .dll and .exe, but the Options passed to the compiler never sets Options.Dll. This means the “Deterministic.dll” case still produces an executable image, so the test isn’t exercising the DLL imageCharacteristics path (and could miss DLL-specific nondeterminism).

 var options = new Options
{
Deterministic = true,
OutputFileName = outputFileName,
};

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• This changes the public API of ILAssembler by replacing the public Compile return type (PEBuilder?) with a new public CompilationResult and updating the tuple signatures. The PR description doesn’t link an api-approved issue, so this can’t be validated against an approved API shape per the repo’s API approval process. Either (a) link an approved issue, or (b) keep the public surface unchanged and move the new behavior behind internal APIs / internal helper types.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[Copilot]

Pull request overview

Copilot reviewed 25 out of 29 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tools/ilasm/tests/ILAssembler.Tests/GenericTests.cs:167

• GenericType_MoreThanMetadataIndexRange_IsAcceptedForCompatibility builds a very large IL source string (65,537 generic parameters). Initializing StringBuilder without an estimated capacity will cause repeated growth reallocations and extra copying, which can significantly slow this test and increase memory usage in CI.

Consider supplying an initial capacity that scales with GenericParameterCount to keep this test cheaper while preserving coverage.

 StringBuilder source = new("""
.assembly extern mscorlib { }
.assembly test { }
.class public auto ansi Test<
""");

[Copilot]

Pull request overview

Copilot reviewed 26 out of 30 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• DocumentCompiler is public, and this change replaces its public Compile(...) return type from PEBuilder? to CompilationResult? (and adds a new public type). That’s a source-breaking public API change for any out-of-repo consumers of ILAssembler, and it also triggers the repo’s “new public API requires api-approved issue” process.

If ILAssembler isn’t intended to expose a stable public API, consider making DocumentCompiler (and/or the new CompilationResult) internal instead. If it is intended as public API, please link an api-approved issue in the PR description and ensure the API shape is approved before merging.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[jkoritzinsky]

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

Note

This PR description was generated with GitHub Copilot.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[jkoritzinsky]

/azp run runtime-coreclr ilasm

[azure-pipelines]

Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

This PR updates the runtime test pipeline template to run an additional ILASM round-trip scenario that uses the managed ILASM implementation when the ilasm test group is selected.

Changes:

• Adds managedilasmroundtrip to the ilasm test group’s Helix scenarios list (alongside ilasmroundtrip).

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 25 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/tools/ilasm/tests/ILAssembler.Tests/AssemblyTests.cs:349

• ManagedIlasm_DeterministicOutput_IsByteIdentical varies the output file extension between .dll and .exe, but the Options passed to the compiler never sets Dll. That means the ".dll" case still produces an image whose COFF Characteristics might not have the DLL bit set, so the test doesn't actually exercise the new Options.Dll behavior and can mask regressions in DLL-vs-EXE emission.

 var options = new Options
{
Deterministic = true,
OutputFileName = outputFileName,
};

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• DocumentCompiler.Compile changes its public return type from PEBuilder? to CompilationResult?. Since DocumentCompiler is public, this is a breaking change for any external consumers of ILAssembler, and it also introduces/expands public surface area (CompilationResult). If ILAssembler isn't intended to be a supported public API, consider making the types/members internal instead; otherwise, consider keeping the existing API shape (e.g., retaining a PEBuilder?-returning entry point) to avoid breaking downstream code.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[Copilot]

Pull request overview

Copilot reviewed 22 out of 26 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/tools/ilasm/tests/ILAssembler.Tests/AssemblyTests.cs:410

• The deterministic-output test varies the output file name between .dll and .exe, but the Options passed to the compiler never sets Options.Dll. This means the “Deterministic.dll” case still produces an executable image, so the test isn’t exercising the DLL imageCharacteristics path (and could miss DLL-specific nondeterminism).

 var options = new Options
{
Deterministic = true,
OutputFileName = outputFileName,
};

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• This changes the public API of ILAssembler by replacing the public Compile return type (PEBuilder?) with a new public CompilationResult and updating the tuple signatures. The PR description doesn’t link an api-approved issue, so this can’t be validated against an approved API shape per the repo’s API approval process. Either (a) link an approved issue, or (b) keep the public surface unchanged and move the new behavior behind internal APIs / internal helper types.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[Copilot]

Pull request overview

Copilot reviewed 25 out of 29 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tools/ilasm/tests/ILAssembler.Tests/GenericTests.cs:167

• GenericType_MoreThanMetadataIndexRange_IsAcceptedForCompatibility builds a very large IL source string (65,537 generic parameters). Initializing StringBuilder without an estimated capacity will cause repeated growth reallocations and extra copying, which can significantly slow this test and increase memory usage in CI.

Consider supplying an initial capacity that scales with GenericParameterCount to keep this test cheaper while preserving coverage.

 StringBuilder source = new("""
.assembly extern mscorlib { }
.assembly test { }
.class public auto ansi Test<
""");

[Copilot]

Pull request overview

Copilot reviewed 26 out of 30 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• DocumentCompiler is public, and this change replaces its public Compile(...) return type from PEBuilder? to CompilationResult? (and adds a new public type). That’s a source-breaking public API change for any out-of-repo consumers of ILAssembler, and it also triggers the repo’s “new public API requires api-approved issue” process.

If ILAssembler isn’t intended to expose a stable public API, consider making DocumentCompiler (and/or the new CompilationResult) internal instead. If it is intended as public API, please link an api-approved issue in the PR description and ensure the API shape is approved before merging.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[jkoritzinsky]

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

Note

This PR description was generated with GitHub Copilot.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[jkoritzinsky]

/azp run runtime-coreclr ilasm

[azure-pipelines]

Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

This PR updates the runtime test pipeline template to run an additional ILASM round-trip scenario that uses the managed ILASM implementation when the ilasm test group is selected.

Changes:

• Adds managedilasmroundtrip to the ilasm test group’s Helix scenarios list (alongside ilasmroundtrip).

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 25 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/tools/ilasm/tests/ILAssembler.Tests/AssemblyTests.cs:349

• ManagedIlasm_DeterministicOutput_IsByteIdentical varies the output file extension between .dll and .exe, but the Options passed to the compiler never sets Dll. That means the ".dll" case still produces an image whose COFF Characteristics might not have the DLL bit set, so the test doesn't actually exercise the new Options.Dll behavior and can mask regressions in DLL-vs-EXE emission.

 var options = new Options
{
Deterministic = true,
OutputFileName = outputFileName,
};

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• DocumentCompiler.Compile changes its public return type from PEBuilder? to CompilationResult?. Since DocumentCompiler is public, this is a breaking change for any external consumers of ILAssembler, and it also introduces/expands public surface area (CompilationResult). If ILAssembler isn't intended to be a supported public API, consider making the types/members internal instead; otherwise, consider keeping the existing API shape (e.g., retaining a PEBuilder?-returning entry point) to avoid breaking downstream code.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[Copilot]

Pull request overview

Copilot reviewed 22 out of 26 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/tools/ilasm/tests/ILAssembler.Tests/AssemblyTests.cs:410

• The deterministic-output test varies the output file name between .dll and .exe, but the Options passed to the compiler never sets Options.Dll. This means the “Deterministic.dll” case still produces an executable image, so the test isn’t exercising the DLL imageCharacteristics path (and could miss DLL-specific nondeterminism).

 var options = new Options
{
Deterministic = true,
OutputFileName = outputFileName,
};

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• This changes the public API of ILAssembler by replacing the public Compile return type (PEBuilder?) with a new public CompilationResult and updating the tuple signatures. The PR description doesn’t link an api-approved issue, so this can’t be validated against an approved API shape per the repo’s API approval process. Either (a) link an approved issue, or (b) keep the public surface unchanged and move the new behavior behind internal APIs / internal helper types.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[Copilot]

Pull request overview

Copilot reviewed 25 out of 29 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tools/ilasm/tests/ILAssembler.Tests/GenericTests.cs:167

• GenericType_MoreThanMetadataIndexRange_IsAcceptedForCompatibility builds a very large IL source string (65,537 generic parameters). Initializing StringBuilder without an estimated capacity will cause repeated growth reallocations and extra copying, which can significantly slow this test and increase memory usage in CI.

Consider supplying an initial capacity that scales with GenericParameterCount to keep this test cheaper while preserving coverage.

 StringBuilder source = new("""
.assembly extern mscorlib { }
.assembly test { }
.class public auto ansi Test<
""");

[Copilot]

Pull request overview

Copilot reviewed 26 out of 30 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• DocumentCompiler is public, and this change replaces its public Compile(...) return type from PEBuilder? to CompilationResult? (and adds a new public type). That’s a source-breaking public API change for any out-of-repo consumers of ILAssembler, and it also triggers the repo’s “new public API requires api-approved issue” process.

If ILAssembler isn’t intended to expose a stable public API, consider making DocumentCompiler (and/or the new CompilationResult) internal instead. If it is intended as public API, please link an api-approved issue in the PR description and ensure the API shape is approved before merging.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[jkoritzinsky]

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

Note

This PR description was generated with GitHub Copilot.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[jkoritzinsky]

/azp run runtime-coreclr ilasm

[azure-pipelines]

Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

This PR updates the runtime test pipeline template to run an additional ILASM round-trip scenario that uses the managed ILASM implementation when the ilasm test group is selected.

Changes:

• Adds managedilasmroundtrip to the ilasm test group’s Helix scenarios list (alongside ilasmroundtrip).

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 25 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/tools/ilasm/tests/ILAssembler.Tests/AssemblyTests.cs:349

• ManagedIlasm_DeterministicOutput_IsByteIdentical varies the output file extension between .dll and .exe, but the Options passed to the compiler never sets Dll. That means the ".dll" case still produces an image whose COFF Characteristics might not have the DLL bit set, so the test doesn't actually exercise the new Options.Dll behavior and can mask regressions in DLL-vs-EXE emission.

 var options = new Options
{
Deterministic = true,
OutputFileName = outputFileName,
};

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• DocumentCompiler.Compile changes its public return type from PEBuilder? to CompilationResult?. Since DocumentCompiler is public, this is a breaking change for any external consumers of ILAssembler, and it also introduces/expands public surface area (CompilationResult). If ILAssembler isn't intended to be a supported public API, consider making the types/members internal instead; otherwise, consider keeping the existing API shape (e.g., retaining a PEBuilder?-returning entry point) to avoid breaking downstream code.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[Copilot]

Pull request overview

Copilot reviewed 22 out of 26 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/tools/ilasm/tests/ILAssembler.Tests/AssemblyTests.cs:410

• The deterministic-output test varies the output file name between .dll and .exe, but the Options passed to the compiler never sets Options.Dll. This means the “Deterministic.dll” case still produces an executable image, so the test isn’t exercising the DLL imageCharacteristics path (and could miss DLL-specific nondeterminism).

 var options = new Options
{
Deterministic = true,
OutputFileName = outputFileName,
};

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• This changes the public API of ILAssembler by replacing the public Compile return type (PEBuilder?) with a new public CompilationResult and updating the tuple signatures. The PR description doesn’t link an api-approved issue, so this can’t be validated against an approved API shape per the repo’s API approval process. Either (a) link an approved issue, or (b) keep the public surface unchanged and move the new behavior behind internal APIs / internal helper types.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{

[Copilot]

Pull request overview

Copilot reviewed 25 out of 29 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tools/ilasm/tests/ILAssembler.Tests/GenericTests.cs:167

• GenericType_MoreThanMetadataIndexRange_IsAcceptedForCompatibility builds a very large IL source string (65,537 generic parameters). Initializing StringBuilder without an estimated capacity will cause repeated growth reallocations and extra copying, which can significantly slow this test and increase memory usage in CI.

Consider supplying an initial capacity that scales with GenericParameterCount to keep this test cheaper while preserving coverage.

 StringBuilder source = new("""
.assembly extern mscorlib { }
.assembly test { }
.class public auto ansi Test<
""");

[Copilot]

Pull request overview

Copilot reviewed 26 out of 30 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tools/ilasm/src/ILAssembler/DocumentCompiler.cs:21

• DocumentCompiler is public, and this change replaces its public Compile(...) return type from PEBuilder? to CompilationResult? (and adds a new public type). That’s a source-breaking public API change for any out-of-repo consumers of ILAssembler, and it also triggers the repo’s “new public API requires api-approved issue” process.

If ILAssembler isn’t intended to expose a stable public API, consider making DocumentCompiler (and/or the new CompilationResult) internal instead. If it is intended as public API, please link an api-approved issue in the PR description and ensure the API shape is approved before merging.

public sealed class DocumentCompiler
{
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(SourceText document, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{
return Compile([document], includedDocumentLoader, resourceLocator, options);
}
public (ImmutableArray<Diagnostic>, CompilationResult?) Compile(ImmutableArray<SourceText> documents, Func<string, SourceText> includedDocumentLoader, Func<string, byte[]> resourceLocator, Options options)
{