Fix FindPublicMethodBySignature matching static methods for instance calls

[lewing]

Summary

FindPublicMethodBySignature (introduced in #14079) uses _receiverType.GetMethods() without binding flags, which returns all public methods including static ones. When resolving an instance call like String.Equals with types [string, string], it incorrectly matches the static String.Equals(String, String) overload instead of falling through to CoerceArguments which would parse the enum string argument ("StringComparison.InvariantCulture") correctly.

The old Type.GetMethod(name, bindingFlags, null, types, null) respected BindingFlags.Instance and excluded static methods, so it returned null and correctly fell through to enum coercion.

Fix

Pass _bindingFlags to GetMethods() so it filters by instance/static correctly, matching the behavior of the replaced code.

Reproduction

The bug breaks any MSBuild condition using instance methods with enum parameters passed as strings. For example, runtime's Directory.Build.targets:

Condition="!$(TargetOS.Equals($(TargetOS.ToLower()), StringComparison.InvariantCulture))"

This fires an error claiming TargetOS='linux' is not lowercase, because String.Equals("linux", "StringComparison.InvariantCulture") (static, ordinal comparison) returns false.

Reproduced locally by building MSBuild from commit 161f9ea and running against a minimal test project. Fix verified.

Impact

This regression blocks the msbuild flow PR to the VMR (dotnet/dotnet#7406) — the SB_CentOSStream10_Offline leg fails because it's the first consumer of the newly-built MSBuild.

Fixes #14190

[Copilot]

Pull request overview

Fixes a regression in property-function late-bound method resolution where instance calls could incorrectly match static overloads due to unfiltered reflection enumeration.

Changes:

• Update FindPublicMethodBySignature to enumerate methods via _receiverType.GetMethods(_bindingFlags) so instance/static filtering matches the original GetMethod(..., bindingFlags, ...) behavior.

[github-actions]

[MODERATE] Correctness — related GetMethods() without BindingFlags at line 1247

The LateBindExecute CoerceArguments fallback (line 1247, also introduced in #14079) has the same unguarded GetMethods() call that this PR fixes in FindPublicMethodBySignature:

// line 1247 — unchanged in this PR
members = _receiverType.GetMethods().Where(m => string.Equals(m.Name, _methodMethodName, StringComparison.OrdinalIgnoreCase));

Type.GetMethods() without BindingFlags is equivalent to GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static) — it returns all public methods, both instance and static. For an instance call, the else branch here could enumerate a static overload before the intended instance overload. If CoerceArguments accepts the static match first (e.g. static String.Equals(String, String) matches any two string arguments trivially), it invokes the wrong overload.

Note that the IntrinsicFunctions branch just above correctly passes bindingFlags (the local parameter, which equals _bindingFlags at the call site) to FindMembers. The else branch should do the same.

Recommendation: Replace _receiverType.GetMethods() with _receiverType.GetMethods(bindingFlags) at line 1247 to consistently filter by instance/static, matching both sibling branches and the fix in this PR.

Generated by Expert Code Review (on open) for issue #14191 · 2.1K AIC · ⊞ 29.9K · ◷

[github-actions]

Follow-up issues from PR #14079 not addressed in this fix

Three sibling concerns, all tracing back to the same PR #14079 that introduced the regression, are still present after this fix.

---

1. Stale <see cref> in doc comment (line 1160) — NIT

/// parameter-type signature, using the public-only <see cref="Type.GetMethods()"/> overload.

After the fix the code calls GetMethods(_bindingFlags) (the BindingFlags overload), but the <see cref> still resolves to the no-arg overload in IntelliSense/tooling and the phrase "public-only" no longer accurately describes the scope — the whole point of the fix is that _bindingFlags now governs instance-vs-static selection.

Suggestion: Update the summary to reference Type.GetMethods(BindingFlags) and note that _bindingFlags determines whether instance or static methods are in scope.

---

2. Two more GetMethods() calls without _bindingFlags (lines 515 and 1247) — MODERATE

Same root-cause as the fixed line — introduced by PR #14079, not addressed here:

Line 515 (out-parameter path):

IEnumerable<MethodInfo> methods = _receiverType.GetMethods()
    .Where(m => m.Name.Equals(_methodMethodName) && m.GetParameters().Length == args.Length);

Line 1247 (LateBindExecute fallback, CoerceArguments branch):

members = _receiverType.GetMethods()
    .Where(m => string.Equals(m.Name, _methodMethodName, StringComparison.OrdinalIgnoreCase));

Both issues:

• Correctness: _bindingFlags carries Instance or Static at call time (set unconditionally in Execute() before this path is reached); omitting it means static overloads remain candidates when dispatching instance calls and vice-versa, which is the same class of bug this PR fixes.
• Performance: Expander.cs is a hot path called per-evaluation. Each call allocates a full MethodInfo[] from GetMethods() plus a LINQ IEnumerable<MethodInfo> wrapper from .Where(). Replacing with GetMethods(_bindingFlags) and a plain foreach eliminates both allocations.

Recommendation: File a follow-up issue referencing this PR and #14079 to apply GetMethods(_bindingFlags) consistently at lines 515 and 1247 and replace the LINQ .Where() with a plain loop, consistent with the fix applied in FindPublicMethodBySignature.

Generated by Expert Code Review (on open) for issue #14191 · 2.1K AIC · ⊞ 29.9K · ◷

[github-actions]

Code Review — PR #14191

The fix is correct: _bindingFlags always has either BindingFlags.Static or BindingFlags.Instance set (and BindingFlags.Public, never BindingFlags.NonPublic) by the time LateBindExecute calls FindPublicMethodBySignature, so GetMethods(_bindingFlags) correctly filters to instance-only or static-only methods and exactly matches the behavior of the original GetMethod(..., bindingFlags, ...) call that was replaced in #14079.

#	Dimension	Verdict
4	Test Coverage	🟠 1 MAJOR
18	Documentation Accuracy	🟡 1 NIT
22	Correctness (related GetMethods() at line 1247)	🟡 1 MODERATE

✅ 21/24 dimensions clean.

• Test Coverage — no regression test for this bug fix
• Correctness — LateBindExecute line 1247 has the same unguarded GetMethods() from #14079; should use bindingFlags parameter (see inline comment on PR)
• Docs — XML doc for FindPublicMethodBySignature still references the parameterless overload

Generated by Expert Code Review (on open) for issue #14191 · 2.1K AIC · ⊞ 29.9K

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[lewing]

@Forgind it looks like this compare has been case sensitive since it was introduced, is this worth addressing here or in a follow up?