Skip to content

Make R2R single method mode compatible with composite mode - #118043

Open
kg wants to merge 1 commit into
dotnet:mainfrom
kg:issue109200
Open

kg wants to merge 1 commit into
dotnet:mainfrom
kg:issue109200

Conversation

@kg

@kg kg commented Jul 24, 2025 •

Copy link
Copy Markdown
Contributor

This allows researching composite R2R failures more easily

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@kg kg reopened this Aug 30, 2025
@kg kg changed the title Investigational changes for issue #109200 Make R2R single method mode compatible with composite mode Sep 24, 2025
@kg kg removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Sep 24, 2025
@kg
kg marked this pull request as ready for review September 24, 2025 01:52
Copilot AI review requested due to automatic review settings September 24, 2025 01:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@JulieLeeMSFT

Copy link
Copy Markdown
Member

@davidwrighton, do you think it's worth finishing this PR?

@github-actions

github-actions Bot commented Jul 19, 2026 •

Copy link
Copy Markdown
Contributor

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
    }
  ]
}

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants