Fix compiler crashes when calling inherited base method on external generic IL - #20272
Conversation
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
Warning No PR link found in some release notes, please consider adding it.
|
T-Gro
left a comment
There was a problem hiding this comment.
Nice, focused fix — guarding the resolve with an explicit existence check is the right shape, and it reads much better than the old blanket try/with _ -> (). Two things worth addressing before merge, one of them important.
The regression test doesn't exercise the bug
In the test, OnDetaching is declared directly on the generic Behavior<T>:
public class Behavior<T> { public virtual void OnDetaching() { } }But the crash in #20264 only happens when the method is inherited and therefore absent from the immediate generic type's own metadata. ILTypeDef.Methods.FindByName (and the resolveILMethodRefWithRescope lookup it feeds) only searches the type's own method table — it doesn't walk the base chain. When OnDetaching sits on Behavior<T> itself, the pre-fix resolveILMethodRefWithRescope finds it and never reaches the failwith, so this test compiles cleanly with or without the fix. It passes today, but it wouldn't have failed before the patch — so it doesn't guard against regression.
To actually reproduce the reported crash, the method needs to live one level up, matching the real Behavior<T> : Behavior shape:
namespace External
{
public class BehaviorBase { public virtual void OnDetaching() { } }
public class Behavior<T> : BehaviorBase { }
}With that C#, the pre-fix compiler throws no method named OnDetaching found in type ...Behavior1and the post-fix compiler succeeds — which is the behavior this PR is meant to lock in. Worth confirming the test fails onmain` before the source change lands.
Removing the try/with narrows the crash class but doesn't close it
The old code caught every failwith inside resolveILMethodRefWithRescope. The new baseMethodExists guard only prevents the first one (empty name+arity match). The two later failwiths remain reachable and are now unguarded: if a same-name/same-arity method exists on the immediate type but the full signature filter (calling convention, rescoped arg/return types, generic arity) yields [] or multiple matches — e.g. an overload whose signature differs while the genuinely-called method is inherited — resolveILMethodRefWithRescope will throw straight through. It's a narrow window, but it's the same failure mode #20264 reported. Since there's no non-throwing resolve variant, either keep a defensive try/with around the resolve, or check the full signature (not just name+arity) before calling it.
Minor
FindByName name |> List.exists (fun x -> List.length x.Parameters = argCount) is exactly not (isNil (Methods.FindByNameAndArity(name, argCount))) — the existing FindByNameAndArity helper already does the name+arity filter, so calling it keeps this in step with resolveILMethodRefWithRescope's own lookup.
One behavioral note to be aware of (not necessarily a change request): skipping the check when the method isn't on the immediate type means a call to a genuinely abstract inherited base method no longer produces tcCannotCallAbstractBaseMember. Trading a hard crash for a missing diagnostic is fine; walking the base chain (option 1 in the issue) would preserve it if that ever matters.
39f5c6c to
974be18
Compare
974be18 to
039b15b
Compare
…FindByNameAndArity, and keep a try/with around signature matching. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
|
||
| if mdef.IsAbstract then | ||
| errorR(Error(FSComp.SR.tcCannotCallAbstractBaseMember(RichText.mkMethod mdef.Name), m)) | ||
| with _ -> |
There was a problem hiding this comment.
Not sure if it a good idea. Advise what to do here
|
The 20264 test now declares |
Summary
Fixes #20264 by guarding the IL abstract-base check so it only applies when the raw metadata type actually declares the method being called. This avoids the compiler crash when the method is inherited from an external generic base type, such as
Microsoft.Xaml.Interactivity.Behavior<T>. The check now skips the unsafe resolve path instead of throwing an unhandled exception.Changes
src/Compiler/Checking/PostInferenceChecks.fsbefore resolving an IL base method on the raw metadata type.base.OnDetaching()on an inherited generic external base method.docs/release-notes/.FSharp.Compiler.Service/11.0.100.mdfor issue F# compiler crashes when calling inherited base method on external generic IL behavior type (OnDetaching/Behavior<T>) #20264.