luau icon indicating copy to clipboard operation
luau copied to clipboard

Function overloads using string singleton arg and fallback `any` always uses fallback

Open JohnnyMorganz opened this issue 2 years ago • 3 comments

Consider the following code:

local foo: ((val: "a") -> number) & ((val: "b") -> string) & ((val: any) -> boolean) = nil
 
local x = foo("a")

The type of x will always be boolean, even though it matches the first overload in the list, and we want it to be number.

I did a little bit of digging, with the culprit being around this area: https://github.com/Roblox/luau/blob/0d6aacf407d25d547ddc93266005e33365f82403/Analysis/src/TypeInfer.cpp#L4114-L4151

It seems that if I remove the final overload which allows any to be an argument, then expected types will list {"a", "b"} as singletons. If I then introduce the any overload, expected types will just be any.

This then influences what argPack is. When expected types is {"a", "b"}, then argPack is the string singleton "a". If, however, expected types is just any, then the argPack is just the primitive String type, not the more correct string singleton. With this, we then fail to unify when we check the first overload, so it falls back to the last overload.

JohnnyMorganz avatar Mar 23 '23 16:03 JohnnyMorganz

Looking further, there seems to be a few possible ways this could be fixed:

  • Setting forceSingleton to true during this call in checkExprList: https://github.com/Roblox/luau/blob/0d6aacf407d25d547ddc93266005e33365f82403/Analysis/src/TypeInfer.cpp#L4562 This fixes the issue, but does cause a lot of tests to fail since error messages are changed since a more strict singleton type is now present. I'm not sure if this is acceptable or not
  • Making an any type return true in maybeSingleton, so that checkExpr creates a singleton type instead of a generic string type. This has the same "problem" as above
  • Make getExpectedTypesForCall to return "a" | "b" | any rather than normalising to any. Not sure this is possible.

JohnnyMorganz avatar Mar 23 '23 21:03 JohnnyMorganz

Actually... there seems to be another solution: changing the last overload to be (val: unknown) -> boolean. This solves the original task, without having the drawback of any causing problems during unification, causing this issue.

I want to use this in the context of require, which is typed as declare function require(target: any): any. Maybe this could be changed to target: unknown. Unsure on any consequences for this though.

JohnnyMorganz avatar Mar 23 '23 21:03 JohnnyMorganz

Part of the problem is that a function

  f : ((S1) -> T1) & ((S2) -> T2)

is treated the same as

  f : ((S1) -> T1) & ((S2) -> T2) & ((S1 & S2) -> (T1 & T2)) 

and as

  f : ((S1) -> T1) & ((S2) -> T2) & ((S1 | S2) -> (T1 I T2)) 

You can see the loooooong discussion of why at https://github.com/Roblox/luau/blob/42a2805f85d2fff8360a8da9b7af33f30c322430/Analysis/src/Normalize.cpp#L2267

So your original function:

((val: "a") -> number) & ((val: "b") -> string) & ((val: any) -> boolean) 

ought to treated as also having overloads

("a" & any) -> (number & boolean)

which is the same as

("a") -> never

We have "overhaul function overload resolution" on our roadmap, but in this case you'd get x having type number & boolean which is probably not what you want.

asajeffrey avatar Mar 23 '23 21:03 asajeffrey