Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't count failed signature matches as autocast matches #12337

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

HertzDevil
Copy link
Contributor

Supersedes #10701. The following description is adopted from that PR.

Consider the following:

def foo(x : Int8, y : Char); end
def foo(x : UInt8, y : String); end

foo(1, 'a')       # Error: ambiguous call, implicit cast of 1 matches all of Int8, UInt8
foo(y: 'a', x: 1) # okay

def bar(x : Char, y : Int8); end
def bar(x : String, y : UInt8); end

bar('a', 1)       # okay
bar(y: 1, x: 'a') # Error: ambiguous call, implicit cast of 1 matches all of Int8, UInt8

Each overload set is checked in that order, because neither overload is more restricted than the other. What happens here is:

  • Neither overload matches when autocasting is disabled.
  • Method lookup is repeated with autocasting enabled, and proceeds to check argument compatibility following argument order.
  • 1 successfully matches Int8 because 1 is within Int8's range. The compiler adds this to a list of partial autocast matches.
  • 'a' successfully matches Char, so there is a successful signature match. But the compiler must also check other defs to detect ambiguous autocasts.
  • 1 successfully matches UInt8 because 1 is within UInt8's range. The partial autocast match list now contains both integer types.
  • 'a' does not match String, so this signature match fails, but the compiler does not reset the list of partial autocast matches. Thus 1 is considered to be ambiguous, even though the UInt8 autocast match is associated with a call that fails.

This PR makes it so that autocast matches are added only after a signature match succeeds completely (with AutocastType#add_autocast_matches). Thus the autocasting behavior is no longer dependent on the argument order, and both error calls above become unambiguous:

def foo(x : Int8, y : Char); end
def foo(x : UInt8, y : String); end

foo(1, 'a') # okay, matches first overload

def bar(x : Char, y : Int8); end
def bar(x : String, y : UInt8); end

bar('a', 1) # okay, matches first overload

Does not fix #8600 (review), since in that case all signature matches succeed.

Note that splat restrictions never autocast:

def foo(*x : *{Int8}); end

foo(1) # Error: no overload matches

So two of the calls to Crystal::Type#restrict inside Crystal::CallSignature#match are not associated with any autocast checks.

@HertzDevil HertzDevil added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic labels Jul 30, 2022
end
end

def add_autocast_matches(other : ASTNode, context)
Copy link
Member

Choose a reason for hiding this comment

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

When does this case trigger? Shouldn't this be an error?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants