luau-lang / luau

A fast, small, safe, gradually typed embeddable scripting language derived from Lua
https://luau.org
MIT License
3.98k stars 373 forks source link

Autocompleting OO-Style methods is not working in the new type solver. #1239

Closed karl-police closed 2 months ago

karl-police commented 5 months ago

My testing code that I put inside Autocomplete.test.cpp

TEST_CASE_FIXTURE(ACBuiltinsFixture, "idk2")
{
    // Change this to "false" to compare it to the old version
    ScopedFastFlag sff[]{
        {FFlag::DebugLuauDeferredConstraintResolution, true},
    };

    auto check1 = check(R"(
local name = {}
name.__index = name

function name.new(abc)
    local obj = setmetatable({}, name)
    obj.a = abc

    return obj
end

function name:test()
    self.b = "no"
end

local newClass = name.new("e")
newClass:@1
)");

    auto ac1 = autocomplete('1');
    CHECK(ac1.entryMap.count("test"));
}

Description

As you see DebugLuauDeferredConstraintResolution is enabled here. And what happens is that CHECK(ac1.entryMap.count("test")) fails due to that.

I tried to figure out why, and figured out, sort of a cause, but not an exact one, because there's too much going on. I wish I found more.

It has to do with this, though I am surprised to have never noticed this, but I barely test with DebugLuauDeferredConstraintResolution https://github.com/luau-lang/site/commit/a48d8494fce108882656f3c3b3f01d2386fa1c92

  Cause starts here: https://github.com/luau-lang/luau/blob/259e50903855d1b8be79edc40fc275fd04c9c892/Analysis/src/Frontend.cpp#L1372

If DebugLuauDeferredConstraintResolution is enabled, it will use Luau::check, (which is where the issue is at) https://github.com/luau-lang/luau/blob/259e50903855d1b8be79edc40fc275fd04c9c892/Analysis/src/Frontend.cpp#L1381-L1384

If it's not enabled, it will use the previous typeChecker https://github.com/luau-lang/luau/blob/259e50903855d1b8be79edc40fc275fd04c9c892/Analysis/src/Frontend.cpp#L1411

Now... I know, that this is literally what DebugLuauDeferredConstraintResolution is about... But that's what I figured out.

 

Expected Result

You will get :test

Actual Result

It seems like that only functions are affected, for some reason. You're forced to give the function type syntax... Generic types however won't work either. However, properties seem to recognise the type from generic types, just not functions within that one class table.

 

 

 

Research

I tried to find workarounds. I combo'd it like so as example: image

But I had to comment this out https://github.com/luau-lang/luau/blob/master/Analysis/src/Unifier.cpp#L407

But that just uses the old type checker....... but it's at that area, uhhh.... probably not really a special find. But Unifier supported it.

I don't even know how this is failing, it works fine outside that test. Unsure, I am probably enabling fast flags wrong. image

karl-police commented 5 months ago

Maybe this comment tells more.

image

karl-police commented 5 months ago

It's because abc doesn't receive a type. It receives a freetype but idk, it needs to be a non-free type for this to be fixed. This is very very confusing.

HOW

does obj.a = abc receive the type string but the argument itself not...

karl-police commented 5 months ago

Hi @karl-police ! I noticed in your test you wrote:

local newClass = name.new("e")
newClass:@1

In this case, name.new passes "e" as the receiver of the new method. name:new("abc") is effectively name.new(self, "abc"). When I replace name.new with name:new, this test works.

There's an issue here though.

I did not write name.new(self, abc) the defined function is name.new(abc) as the constructor.

Doing name:new in this case, is going to lead to bad results. The fact that the it somehow makes it through the type check is interesting.

karl-police commented 5 months ago

I found out that the issue could be somewhere around here

https://github.com/luau-lang/luau/blob/af15c3cf17ed211f7f4a257cbc5bccd298c2e9e0/Analysis/src/ConstraintGenerator.cpp#L2781-L2790

freshTypes work but it's very dank I do not understand how I am supposed to keep track of changes made because it's all dank, help

 

That's actually the only issue. If I can understand how this free fresh type gets modified to an actual type e.g. obj.a getting string,

BUT I DO NOT understand how it gets it, because I don't know how it works, there's like a bunch of AST and pointers and idk.

The type of abc is a in this case, idk what that should be

karl-police commented 5 months ago

I don't know what happens to the type of abc, I see that a always gets string, uhhh...

I don't know how I can check the value of abc after the check result properly

Vighnesh-V commented 5 months ago

Hi! I think this is a bug on our end. We are working on making sure the experimental new type solver (under the DebugLuauDeferredConstraintResolution) is more stable, but it is not guaranteed to be at parity with the old solver. I have filed this as a bug and we will fix it at our earliest convenience. @karl-police Is there a particular reason you need the old new solver?

karl-police commented 5 months ago

Hi! I think this is a bug on our end. We are working on making sure the experimental new type solver (under the DebugLuauDeferredConstraintResolution) is more stable, but it is not guaranteed to be at parity with the old solver. I have filed this as a bug and we will fix it at our earliest convenience. @karl-police Is there a particular reason you need the old solver?

I was just testing a few things. I wanted to combine types together again, I was thinking about making an RFC for variadic generic types, because U... doesn't do anything, not sure.

Then I noticed that something wasn't right with how the autocomplete works, and this is concerning. If THIS doesn't work, then what else might broke from it. That's about that.

karl-police commented 5 months ago

Hi! I think this is a bug on our end. We are working on making sure the experimental new type solver (under the DebugLuauDeferredConstraintResolution) is more stable, but it is not guaranteed to be at parity with the old solver. I have filed this as a bug and we will fix it at our earliest convenience. @karl-police Is there a particular reason you need the ~old~ new solver?

Oh, you edited the old to new.

I don't seek the new solver. I was just using the new solver to see if a future update will break the old stuff. Break, in sense of "heavily".

keyof is cool, but I don't seek it.

I was curious on how I could figure out how to fix this bug here.

But I have not enough knowledge about all of this. Way too many breakpoints, there's gotta be something more efficient.

 

Maybe ConstraintSolver.cpp does it, there's a part where I do see "e" with upperBounds and lowerBounds, though idk how to use AST, maybe that can be used to get the function, then the argument and then somehow override the abc's type or something.

aatxe commented 5 months ago

The sort of questions you're asking in trying to investigate the issue yourself are very architectural ones, and we don't have completed documentation available for the new solver since it is still in production, but in general, free types are how inference actually works, and most things (that do not have immediately determinable types) will be initially assigned a free type. Type inference is implemented between constraint generation (ConstraintGenerator.cpp) and constraint resolution (ConstraintSolver.cpp) where the former produces a pool of constraints along with types (including mostly the initial free types) for everything, and then the latter resolves those constraints to solve the types for the whole program. Free types, in general, have bounds that grow in unification (Unifier2.cpp), and are eventually generalized to their bounds when they have been completely solved.

karl-police commented 4 months ago

Made a post here, where I just figured out something else

https://github.com/luau-lang/luau/discussions/1257

local test = {}
test.__index = test

function test.new(input: string)
    local self = setmetatable({}, test)

    self.name = input
    return self
end

function test:Get()
    return self.name
end

local newTest = test.new("test")

newTest:

This is where :Get() currently shows up, thanks to function test.new(input: string)

But if you mix it with function test.new<free>(input: string)

free is not assigned anywhere, yet will destroy :Get()

But there's something that <free> is doing, and that just reminded me about this issue.

local test = {}
test.__index = test

function test.new<free>()
    local self = setmetatable({}, test)

    self.emptyTable = {}

    return self
end

function test:Get()
    return "nothing"
end

local newTest = test.new()

newTest.emptyTable.AutoComplete = "test"

newTest.emptyTable.

Here, <free> is completly unused, there's no function arguments. However, newTest is now a completly free table. :Get() still doesn't show up in the autocomplete however.

But I can't explain how <free> just magically causes newTest.emptyTable. to autocomplete the contents inside of it.

 

I haven't checked Unifier2.cpp. But it sounds like it fails to merge some content and the object's table. Point is, <free> alone causes this issue already, and I think that's a very good lead to find where the issue is.

karl-police commented 4 months ago

I looked at ConstraintSolver while debugging.

I finally found "test" now I can eventually figure out how it ends up going missing

image

karl-police commented 4 months ago

idk what this is

image

idk how test lands in there, nor do I know where I am at in the ConstraintSolver

karl-police commented 4 months ago

So, when it's not broken, this is the index image

When it is broken, it has a different index, so I am not sure why that may be and what that would mean image

But that info is not useful, because I still haven't found the place where it decides to put it in the entryMap

karl-police commented 3 months ago

Is it possible that this issue was fixed?

aatxe commented 3 months ago

Is it not reproducing now? It's totally possible that this would be fixed unintentionally or as a consequence of some other fixes since we're working hard to polish the new solver.

karl-police commented 3 months ago

Is it not reproducing now? It's totally possible that this would be fixed unintentionally or as a consequence of some other fixes since we're working hard to polish the new solver.

I couldn't repro it, unsure if there are corner cases or if I did something wrong with the flag. But I couldn't get this reproduced.

One of the changelogs had a note somewhere.

karl-police commented 3 months ago

Maybe this one https://github.com/luau-lang/luau/commit/7d4033071abebe09971b410d362c00ffb3084afb

  • Fix error that prevented Class Methods from being overloaded
karl-police commented 2 months ago

@aatxe from what I tested the last time, this bug seems to be fixed, so if that's the case and intended to be fixed awarely

I guess that this issue can be closed? 🤔

However, I'd really specifically like to address these two issues

https://github.com/luau-lang/luau/issues/1308 https://github.com/luau-lang/luau/issues/1309