Fix Edge Case w/ Intersection Type & Math Operation Overloads

[AmberGraceSoftware]

Fixes #1003

Problem:

Currently, there are helper functions in the type inference code which fail to unwrap intersection types when processing math overloads via metamethods.

Here is a simplified repro of this edge case:

--!strict
type BaseClass = typeof(setmetatable(
    {},
    ({} :: any) :: {__add: (BaseClass, BaseClass) -> BaseClass})
)
type SubClass = BaseClass & {extraField: string}

-- OK
local function add1(x: BaseClass, y: BaseClass): BaseClass
    return x + y
end

-- OK
local function add2(x: SubClass, y: BaseClass): BaseClass
    return x + y
end

-- OK
local function add3(x: BaseClass, y: SubClass): BaseClass
    return x + y
end

-- Emits error: "Type Error: (22,9) Type 't1 & {| extraField: string |} where
-- t1 = { @metatable {| __add: (t1, t1) -> t1 |}, {  } }' could not be converted
-- into 'number'; none of the intersection parts are compatible
local function add4(x: SubClass, y: SubClass): BaseClass
    return x + y
end

Solution:

The changes in this PR unwraps the intersection in those edge cases by adding a conditional statement to certain helper functions which try to find the metatable of a given type.

With this PR, the last case (add4) correctly does not emit any errors.

In addition, the error produced when adding an intersected type containing a __add with another invalid type is more accurate, no longer confusing the user by telling them that they should be passing a number type instead.

Use Case:

The use case for this is my library, Dec, which adds inheritance and provides math operation metamethods in a base class for syntax sugar.

Dec is a reactive UI library which has a base class called "Observable", and a number of subclasses such as "State", "Timer", "Spring", etc.

I wanted to add math overloads to the base "Observable" class which map the left and right operands by observing the "sum", "product", etc. of Observable A and Observable B

Documentation of this syntax sugar (Note that this library is still a work in progress):
https://dex.ambergracesoftware.com/docs/Chapter1/State#using-math-operations-on-observables

This is a bit of syntax sugar that I thought would lend itself to some really nice reactive programming syntax; however, I ended up disabling the feature after discovering issues when adding subclasses.

Used to produce error before this PR:

local a = Dec.State(3)
local b = Dec.State(4)
local sum = a + b
print(sum:Current()) -- 7
a:Set(4)
print(sum:Current()) -- 8

Is now, and used to be, OK, due to Observable being the base class:

local a: Dec.Observable<number> = nil
local b: Dec.Observable<number> = nil
local sum = a + b

This may have been a recent regression, since I swear the first code sample worked when I was writing the library, but I may be wrong on it being a regression looking at the git blame.

I'm sort of invested in fixing edge cases like this, but I hope this PR can be a step along the way of making Luau more sound.

---

Code smell to highlight

While this is just a simple fix that addresses my edge case, it seems the code smell here is that there are proliferated helper functions in the first place which do not handle the edge case of intersected types.

How to best solve this is hard to say, other than maybe creating tests whenever an issue is encountered to make sure intersection types get the love they deserve.

[andyfriesen]

Thanks for the contribution!

This is a bit scary because, if we are going to admit the possibility that an intersection might have a metatable, we should also consider the possibility that the intersection has many metatables.

I think it would be acceptable if this function were to return std::nullopt when given an intersection that contains multiple tables with disparate metatables. It's not perfect but it also moves us to a place that's strictly better than where we are now.

The second complication that's likely to rear its head is a cyclic intersection. We try to avoid creating these, but we need to handle them without overflowing the stack when they do crop up.

The usual way we make functions like this resilient in the face of cycles is to write a helper overload that accepts a mutable seen set as an argument. In this case, any intersection that's part of a cycle can safely be said to have no metatable.

std::optional<TypeId> getMetatable(TypeId type, NotNull<BuiltinTypes> builtinTypes, std::set<TypeId>& seen);

std::optional<TypeId> getMetatable(TypeId type, NotNull<BuiltinTypes> builtinTypes)
{
    if (FFlag::LuauIntersectedBinopOverloadFix)
    {
        std::set<TypeId> seen;
        return getMetatable(type, builtinTypes, seen);
    }
    // ...
}

std::optional<TypeId> getMetatable(TypeId type, NotNull<BuiltinTypes> builtinTypes, std::set<TypeId>& seen)
{
    if (seen.count(type))
        return std::nullopt;
    seen.insert(type);
    // ... all the logic from the original getMetatable here ...
}

[AmberGraceSoftware]

Appreciate the review! Thanks for the note on cyclic intersections and other edge cases. I definitely see the issue with intersections of multiple metatable-containing types and how that gets handled. That would need some refactoring outside of the scope of this PR here.

I'll look into the suggestions you mentioned in my free time, hope it can at least be an incremental step towards having fully sound intersection types and make it into production if it's all stable and doesn't make anything worse per se. There was a related issue #983 involving the interaction between metatables and intersection types.

[zeux]

Marking this as draft for now since it requires rebasing and addressing feedback.

[aMiizu]

Any updates on this?

[AmberGraceSoftware]

Any updates on this?

Hello again to the Roblox staff on this—unfortunately I ran out of free time to really address the notes on my implementation for this fix, and I believe there have been a lot of changes to the inference engine since that it would be better to start from scratch at this point.

If you could close this and create a new issue/draft PR with my unit tests cherry picked out that would be great. Fixing this would definitely enable me to do a lot more in terms of math syntax for an OSS library I wrote and maintain, but I don’t currently have the knowhow or time to address this on the refactored type inference system in a way that doesn’t also conflict with roblox’s roadmap.

[aatxe]

In light of your comment, I think it probably makes sense to close this PR out for now. I think it's likely that the existing work on the new type inference engine has done this anyway (since a lot of what that work does is improve the handling of union and intersection types especially). I'll leave the original issue open, of course, until it's verified to be solved, and we would be happy to take a PR for the tests from this PR alone.