Repository navigation
Conversation
|
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the R2R (ReadyToRun) single method mode to work seamlessly with composite mode, making it easier to research composite R2R compilation failures by allowing type lookup across multiple input modules.
Key changes:
- Extends type lookup logic to search across all input modules when in composite mode
- Maintains backwards compatibility with existing single method mode behavior
|
@davidwrighton, do you think it's worth finishing this PR? |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "5505e2b3f0ae2646bbc5f9c6a2be0d7cf668f8ef",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "640fa699439a8518ec995450f131e48220d17ad7",
"last_reviewed_commit": "5505e2b3f0ae2646bbc5f9c6a2be0d7cf668f8ef",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "640fa699439a8518ec995450f131e48220d17ad7",
"last_recorded_worker_run_id": "29676147466",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "5505e2b3f0ae2646bbc5f9c6a2be0d7cf668f8ef",
"review_id": 4730522673
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: crossgen2's single-method mode (--singlemethodtypename etc.) resolves the target type via FindType, which previously only searched the system module. When compiling in composite R2R mode the type of interest frequently lives in one of the other input assemblies, so FindType would throw TypeNotFound and single-method investigation of composite R2R failures was impossible. This PR makes single-method mode usable for researching composite R2R failures.
Approach: FindType still tries the system module first and returns immediately on success. When that fails and --composite is set, it now iterates context.InputFilePaths.Values, resolves each module via the cached GetModuleFromPath, and retries GetTypeByCustomAttributeTypeName against it, returning the first match. The original TypeNotFound exception is preserved as the fall-through when no module contains the type. The refactor from an early throw to an early return keeps behavior identical for the non-composite path.
Summary: This is a small, well-scoped change confined to a developer/diagnostic tool (crossgen2 Program.cs), gated behind the --composite switch, so it cannot affect normal single-file compilation behavior. GetModuleFromPath performs a linear cache lookup and returns already-loaded modules, so the added loop introduces no redundant module loading and negligible cost on an already-infrequent code path. The logic is correct: system module is preferred, input modules are searched only as a fallback, and the terminal exception is unchanged. Passing throwOnFailureToLoad: true is reasonable because these are the tool's own declared input files. No new tests accompany the change, which is acceptable given this is an interactive diagnostic capability with no automated single-method-mode harness; behavior for existing paths is unchanged. LGTM.
Detailed Findings
No blocking or actionable issues. One minor, non-blocking observation: the new loop keeps iterating and re-invoking GetTypeByCustomAttributeTypeName after a module has been resolved; since it returns on the first match this is fine, but if a caller ever passed an ambiguous type name present in multiple composite inputs, the first-input-wins resolution is silent. This matches the pre-existing system-module-first behavior and is appropriate for a diagnostic tool, so no change is required.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 61.9 AIC · ⌖ 10.3 AIC · ⊞ 10K
This allows researching composite R2R failures more easily