Fix false positive in _final send checking for generic classes#4982
Open
SeanTAllen wants to merge 1 commit intomainfrom
Open
Fix false positive in _final send checking for generic classes#4982SeanTAllen wants to merge 1 commit intomainfrom
SeanTAllen wants to merge 1 commit intomainfrom
Conversation
SeanTAllen
added
the
changelog - fixed
ponylang-main
added
the
discuss during sync
The finalisers pass conservatively reports FINAL_MAY_SEND when it encounters a method call on an unknown type. When _final calls a method on a generic class instantiated with concrete type args (e.g., Generic[Prim]), the pass follows into the method body where expressions still reference the generic type parameter A. Since A's constraint is a trait, is_known(A) returns false and the pass reports a false positive. Two changes fix this: 1. Reify the method body with the entity's concrete type arguments before analyzing it. This replaces type parameters with their concrete types so is_known sees the actual type. The recursion guard is set on the original body (not the reified copy) so that recursive lookups correctly detect cycles. 2. Replace ast_get with lookup_try for method resolution. ast_get only searches the entity's own symbol table, missing methods inherited through the provides chain. lookup_try handles the full provides chain. This expands the range of generic code accepted in _final methods. Cases where the concrete type doesn't have the called method in its own scope (e.g., Range calling finite() which exists on the Real trait but not on USize) still fall back to FINAL_MAY_SEND conservatively. Closes #4249
SeanTAllen
force-pushed
the
fix-finaliser-generic-false-positives
branch
from
06236f9 to
b32cd9f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The finalisers pass uses
is_known(type)to decide whether a method call might send a message. When_finalcalls a method on a generic class with concrete type args (likeGeneric[Prim]), the pass follows into the method body where expressions still reference the bare type parameterA. SinceA's constraint is a trait,is_known(A)returns false and the pass conservatively reports the call might send — even though at the call site, the concrete type is fully known and clearly doesn't send anything.Two changes:
Reify the method body with the entity's concrete type arguments before analyzing it. Type parameters get replaced with their concrete types, so
is_knownseesPriminstead ofAand correctly determines no message is sent.Replace
ast_getwithlookup_tryfor method resolution.ast_getonly searches the entity's own symbol table, missing methods inherited through the provides chain.This expands the range of generic code accepted in
_finalmethods. Cases where the concrete type doesn't have the called method in its own scope (e.g., Range callingfinite()which exists on theRealtrait but not onUSize) still produce false positives. The union type false positive mentioned in the issue is also not addressed here.Closes #4249