Remove and clarify PEAssembly constructor parameters

[elinor-fung]

A PEAssembly is either bound by an AssemblyBinder or dynamic (reflection emit). This is pretty unclear from the way the class can be created and the way it is documented.

• Remove redundant PEAssembly::Open and constructor parameters for associated PE image and host assembly. These were only used in one case - and they were derived from a bound assembly - which was equivalent to just passing in the bound assembly.
• Add asserts to the constructor around how its parameters should recommend either bound or dynamic.

cc @dotnet/appmodel @AaronRobinsonMSFT

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 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 simplifies PEAssembly construction to more clearly represent the two supported modes (binder-bound vs reflection-emit), removing a redundant Open overload/constructor parameters and tightening invariants around how instances are created.

Changes:

• Remove PEAssembly::Open(PEImage*, BINDER_SPACE::Assembly*) and related constructor parameters, updating callers to pass the binder-bound assembly directly.
• Add constructor preconditions documenting/enforcing the “bound vs dynamic” split and binder ownership rules.
• Clean up PEAssembly surface by removing IsStrongNamed() and switching IsSystem()/wrappers from BOOL to bool.

Show a summary per file

File	Description
src/coreclr/vm/peassembly.inl	Updates IsSystem() to return bool; removes inline IsStrongNamed() implementation.
src/coreclr/vm/peassembly.h	Updates API/docs to reflect the 2-kind model; removes IsStrongNamed() and the removed Open overload; adjusts constructor signature.
src/coreclr/vm/peassembly.cpp	Implements the simplified constructor/opening logic and adds preconditions; removes the old Open(PEImage*, hostAssembly) path.
src/coreclr/vm/ceeload.h	Updates Module::IsSystem() wrapper to return bool.
src/coreclr/vm/assemblynative.cpp	Updates load path to use PEAssembly::Open(pAssembly) instead of passing PEImage/host separately.
src/coreclr/vm/assembly.hpp	Updates Assembly::IsSystem() wrapper to return bool; removes Assembly::IsStrongNamed() wrapper.

Review details

Suppressed comments (2)

src/coreclr/vm/peassembly.cpp:803

• New code still passes NULL for pointer parameters when constructing PEAssembly. Per native instructions, prefer nullptr for pointer arguments.

Ref: .github/instructions/native.instructions.md ("Prefer nullptr over NULL").

    return new PEAssembly(pBoundAssembly, NULL, /*isSystem*/ true);

src/coreclr/vm/peassembly.h:335

• Constructor default argument uses NULL for a pointer. Native instructions prefer nullptr for pointer constants.

Ref: .github/instructions/native.instructions.md ("Prefer nullptr over NULL").

        AssemblyBinder* pDynamicAssemblyBinder = NULL

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Lite

[dotnet-policy-service]

Tagging subscribers to this area: @agocke, @elinor-fung
See info in area-owners.md if you want to be subscribed.

[Copilot]

Review details

Suppressed comments (1)

src/coreclr/vm/peassembly.cpp:663

• The constructor contract dropped CheckPointer(pEmit, NULL_OK). Keeping this precondition helps catch invalid metadata emitter pointers early and preserves the prior contract checking behavior.

        // A PEAssembly is either bound by an AssemblyBinder or dynamic (reflection emit)
        PRECONDITION((pBoundAssembly == NULL) != (pEmit == NULL));
        // A bound assembly takes its binder from the bind result, not from a caller.
        PRECONDITION(pBoundAssembly == NULL || pDynamicAssemblyBinder == NULL);

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Review details

Suppressed comments (1)

src/coreclr/vm/peassembly.h:23

• Including "../binder/inc/assembly.hpp" from this VM header introduces a heavy binder dependency into common.h’s include chain, which can increase rebuild cost and coupling. The PEAssembly header only needs BINDER_SPACE::Assembly/AssemblyBinder as opaque pointer types; consider forward-declaring them here and moving the binder header include into peassembly.cpp (or another .cpp) that needs the full definition to call GetPEImage()/GetBinder().

#include "sstring.h"
#include "peimage.h"
#include "metadata.h"
#include "../binder/inc/assembly.hpp"
#include "eecontract.h"

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite