FuelLabs / sway

🌴 Empowering everyone to build reliable and efficient smart contracts.
https://docs.fuel.network/docs/sway/
Apache License 2.0
62.64k stars 5.36k forks source link

function composition in impl, method not found #1548

Closed simonr0204 closed 1 year ago

simonr0204 commented 2 years ago

Attempting to compose functions in an impl block results in no method found. E.g.

pub struct A {
    value: u64,
}

impl A {
    fn a(self, a: u64) -> A {
        A {
            value: self.value + a
        }
    }

    fn b(self, b: u64) -> A {
        self.a(b)
    }
}

:

46 |         self.a(b)
   |              ^ No method named "a" found for type "struct A<u64>".

Full repro here: https://github.com/FuelLabs/sway/blob/simon-impl-function-composition-bug/test/src/sdk-harness/test_projects/temp/src/main.sw

adlerjohn commented 2 years ago

This is blocking implementation of more complex stdlib types, so moving to critical priority.

otrho commented 2 years ago

I have taken a look at this and although the problem is quite straight forward, I'm not sure what the solution is.

The problem is while we're type checking the trait methods (i.e., converting them from function declarations to typed function declarations) none of them are in a namespace yet, so they can't refer to each other.

https://github.com/FuelLabs/sway/blob/82ad97ac10560f2339c13fcca9126602da3fc70f/sway-core/src/semantic_analysis/ast_node/mod.rs#L504-L541

The call which fails (or generates the errors) is on line 536.

Initially I thought the solution might be to sort the methods based on dependency like we do for top level functions. But they're not added to the namespace individually so that wouldn't help. They get added as a collection here:

https://github.com/FuelLabs/sway/blob/82ad97ac10560f2339c13fcca9126602da3fc70f/sway-core/src/semantic_analysis/ast_node/mod.rs#L547-L551

So then I thought we could add them to the temporary namespace (impl_namespace in the above code) so that they could type check. But that involves creating TypedFunctionDeclarations for each of the methods first, which is exactly what we're trying to do in the above code anyway. Chicken and egg.

I thought 'dummy' TypedFunctionDeclarations could work, but they still need to have all the TypeInfo values in the FunctionDeclarations converted to TypeId which seems wasteful, plus faking the body and spans and other meta all seems very hacky.

But unless I'm missing a simple path here (I hope I am TBH!) then that may be the only way to fix this. That, or to add the ability to inject very lean non-typechecked function signatures into the namespace temporarily so these guys can type check properly afterwards.

Tagging anyone who's touched the file and might have ideas: @sezna @emilyaherbert @mohammadfawaz @canndrew @mitchmindtree

emilyaherbert commented 2 years ago

That, or to add the ability to inject very lean non-typechecked function signatures into the namespace temporarily so these guys can type check properly afterwards

Put directly, it seems clear to me that the compiler has reached a "critical mass" of issues related to dep ordering / declaration ordering / function references / etc. All of which can be solved by, at a high-level, injecting the compiler with a new phase, between parsing and type checking that "collects" information about modules and declarations. Similar to what you wrote in this issue, I describe this change in detail here: https://github.com/FuelLabs/sway/issues/1578#issuecomment-1133150126

mohammadfawaz commented 2 years ago

@emilyaherbert I'm assuming this is now blocked by the collection context work and the declarations engine? Adding the blocked label for so that no one tries to work on it.

nfurfaro commented 1 year ago

Just curious what the blocker is for this? While working on a chess game, I needed up to 6-7 impl self blocks for a single type to get the lib to compile.

mohammadfawaz commented 1 year ago

The main blocker is the collection context work.

tritao commented 1 year ago

Closed by https://github.com/FuelLabs/sway/commit/13f33f56c108547f18e143e5141cf85b067ab969.