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.

Sign up for GitHub

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

Jump to bottom

Infinite recursion (stack overflow) calling trait function on tuples #5504

Closed
xunilrj opened this issue Jan 22, 2024 · 1 comment · Fixed by #5541 · May be fixed by #5515
Closed

Infinite recursion (stack overflow) calling trait function on tuples #5504

xunilrj opened this issue Jan 22, 2024 · 1 comment · Fixed by #5541 · May be fixed by #5515
Assignees
@esdrubal @vaivaswatha
Labels
P: critical Should be looked at before anything else

Comments

@xunilrj
Copy link
Contributor

xunilrj commented Jan 22, 2024 •
edited

I managed to reproduce this problem with this minimal example on master.

script;

trait T1 {
    fn trait_fn() -> Self;
}

impl T1 for u64 {
    fn trait_fn() -> u64 {
        0
    }
}

impl<A> T1 for (A, ) 
where
    A: T1
{
    fn trait_fn() -> (A, ) {
        (A::trait_fn(), )
    }
}

fn f<T> () 
where
    T: T1
{
    T::trait_fn();
}

fn main() {
    let a = f::<(u64, )>();
}

The first interesting code in the stack is at sway-core/src/language/ty/expression/expression_variant.rs:784 (all the others frames in our codebase point to compiler derived clone()s)

(gdb) where
#0  0x00005555576de047 in alloc::slice::hack::{impl#0}::to_vec<sway_core::language::ty::expression::expression::TyExpression, alloc::alloc::Global> (s=..., 
    alloc=...) at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/alloc/src/slice.rs:124
#1  0x0000555557a16138 in alloc::slice::hack::to_vec<sway_core::language::ty::expression::expression::TyExpression, alloc::alloc::Global> (s=..., alloc=...)
    at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/alloc/src/slice.rs:111
#2  alloc::slice::{impl#0}::to_vec_in<sway_core::language::ty::expression::expression::TyExpression, alloc::alloc::Global> (self=..., alloc=...)
    at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/alloc/src/slice.rs:441
#3  alloc::vec::{impl#10}::clone<sway_core::language::ty::expression::expression::TyExpression, alloc::alloc::Global> (self=0x55555b4b29c0)
    at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/alloc/src/vec/mod.rs:2685
#4  0x0000555557ed22ca in sway_core::language::ty::expression::expression_variant::{impl#11}::clone (self=0x55555b4b29b8)
    at sway-core/src/language/ty/expression/expression_variant.rs:54
#5  0x0000555557b47f08 in sway_core::language::ty::expression::expression::{impl#13}::clone (self=0x55555b4b29b8)
    at sway-core/src/language/ty/expression/expression.rs:24
#6  0x0000555557d3f279 in sway_core::language::ty::ast_node::{impl#22}::clone (self=0x55555b4b29b0) at sway-core/src/language/ty/ast_node.rs:453
#7  0x0000555557d3f058 in sway_core::language::ty::ast_node::{impl#20}::clone (self=0x55555b4b29b0) at sway-core/src/language/ty/ast_node.rs:28
#8  0x0000555557bf582e in alloc::slice::hack::{impl#0}::to_vec<sway_core::language::ty::ast_node::TyAstNode, alloc::alloc::Global> (s=..., alloc=...)
    at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/alloc/src/slice.rs:146
#9  0x0000555557a1a8f8 in alloc::slice::hack::to_vec<sway_core::language::ty::ast_node::TyAstNode, alloc::alloc::Global> (s=..., alloc=...)
    at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/alloc/src/slice.rs:111
#10 alloc::slice::{impl#0}::to_vec_in<sway_core::language::ty::ast_node::TyAstNode, alloc::alloc::Global> (self=..., alloc=...)
--Type <RET> for more, q to quit, c to continue without paging--
    at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/alloc/src/slice.rs:441
#11 alloc::vec::{impl#10}::clone<sway_core::language::ty::ast_node::TyAstNode, alloc::alloc::Global> (self=0x55555b4b30b0)
    at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/alloc/src/vec/mod.rs:2685
#12 0x0000555557be59fa in sway_core::language::ty::code_block::{impl#8}::clone (self=0x55555b4b3088) at sway-core/src/language/ty/code_block.rs:13
#13 0x0000555557c5e8fb in sway_core::language::ty::declaration::function::{impl#19}::clone (self=0x55555b4b3040)
    at sway-core/src/language/ty/declaration/function.rs:30
#14 0x000055555797c6e1 in sway_core::decl_engine::ref::DeclRef<sway_core::decl_engine::id::DeclId<sway_core::language::ty::declaration::function::TyFunctionDecl>>::replace_decls_and_insert_new_with_parent<sway_core::language::ty::declaration::function::TyFunctionDecl> (self=0x7fffff8036b8, decl_mapping=0x7ffffff86b30, 
    handler=0x7fffff804168, ctx=0x7ffffff88290) at sway-core/src/decl_engine/ref.rs:171
#15 0x0000555557b4f76b in sway_core::language::ty::expression::expression_variant::{impl#4}::replace_decls_inner::{closure#0} (handler=0x7fffff804168)
    at sway-core/src/language/ty/expression/expression_variant.rs:784
#16 0x0000555557c6502d in sway_error::handler::Handler::scope<(), sway_core::language::ty::expression::expression_variant::{impl#4}::replace_decls_inner::{closure_env#0}> (self=0x7fffff805228, f=...) at sway-error/src/handler.rs:60
#17 0x0000555557ecdcb4 in sway_core::language::ty::expression::expression_variant::{impl#4}::replace_decls_inner (self=0x55555b327ae0, 
    decl_mapping=0x7ffffff86b30, handler=0x7fffff805228, ctx=0x7ffffff88290) at sway-core/src/language/ty/expression/expression_variant.rs:765
#18 0x0000555557b4e94d in sway_core::decl_engine::replace_decls::ReplaceDecls::replace_decls<sway_core::language::ty::expression::expression_variant::TyExpressionVariant> (self=0x55555b327ae0, decl_mapping=0x7ffffff86b30, handler=0x7fffff805228, ctx=0x7ffffff88290) at sway-core/src/decl_engine/replace_decls.rs:26
#19 0x0000555557b23cce in sway_core::language::ty::expression::expression::{impl#4}::replace_decls_inner (self=0x55555b327ae0, decl_mapping=0x7ffffff86b30, 
    handler=0x7fffff805228, ctx=0x7ffffff88290) at sway-core/src/language/ty/expression/expression.rs:69
@IGI-111 IGI-111 added the P: critical Should be looked at before anything else label Jan 22, 2024
@esdrubal esdrubal self-assigned this Jan 23, 2024
esdrubal added a commit that referenced this issue Jan 24, 2024
@esdrubal
Fixes stack overflow when using recursive trait.
11e19f6 
ReplaceDecls, CollectTypesMetadata, HashWithEngines now have a mechanism that interrupts recursive visiting.

The mechanism works by adding TypeIds or DeclIds to a HashMap or HashSet and no longer visiting TypeIds or DeclIds on the passed collection.

This was necessary because in some cases while visiting a declaration we can go back to the beginning because of recursive DeclIds.

I suspect these changes will also be useful to #3018

This partially fixes #5504, which is still failing because we have yet to fix a recursion problem in the IR side of the compiler.
@esdrubal esdrubal mentioned this issue Jan 24, 2024
Draft
7 tasks
@esdrubal
Copy link
Contributor

So with #5515, ReplaceDecls, CollectTypesMetadata, HashWithEngines now have a mechanism that interrupts recursive visiting.

But we still have a recursion problem in the IR side of the compiler.

Screenshot from 2024-01-24 14-56-44

esdrubal added a commit that referenced this issue Jan 24, 2024
@esdrubal
Fixes stack overflow when using recursive trait.
52ddd02 
ReplaceDecls, CollectTypesMetadata, HashWithEngines now have a mechanism that interrupts recursive visiting.

The mechanism works by adding TypeIds or DeclIds to a HashMap or HashSet and no longer visiting TypeIds or DeclIds on the passed collection.

This was necessary because in some cases while visiting a declaration we can go back to the beginning because of recursive DeclIds.

I suspect these changes will also be useful to #3018

This partially fixes #5504, which is still failing because we have yet to fix a recursion problem in the IR side of the compiler.
esdrubal added a commit that referenced this issue Jan 25, 2024
@esdrubal
Partially fixes stack overflow when using recursive trait.
dfd86b1 
ReplaceDecls, CollectTypesMetadata, HashWithEngines now have a mechanism that interrupts recursive visiting.

The mechanism works by adding TypeIds or DeclIds to a HashMap or HashSet and no longer visiting TypeIds or DeclIds on the passed collection.

This was necessary because in some cases while visiting a declaration we can go back to the beginning because of recursive DeclIds.

I suspect these changes will also be useful to #3018

This partially fixes #5504, which is still failing because we have yet to fix a recursion problem in the IR side of the compiler.
esdrubal added a commit that referenced this issue Feb 2, 2024
@esdrubal
Fixes ReplaceDecls to no longer depend on filter hack.
70994e5 
`ReplaceDecls` was using an hack to replace some function refs based on
the self fuction parameter this didn't work for associated methods.

Now DeclMapping sources also contain a typeid this guarantees that the
replacement is done to the correct declaration in case we have nested
trait methods with the same name replaced.

Fixes #5504
@esdrubal esdrubal mentioned this issue Feb 2, 2024
Merged
7 tasks
esdrubal added a commit that referenced this issue Feb 2, 2024
@esdrubal
Fixes ReplaceDecls to no longer depend on filter hack.
e23f23c 
`ReplaceDecls` was using a hack to replace some function refs based on
the self function parameter didn't work for associated methods.

Now DeclMapping sources also contain a typeid that guarantees that the
replacement is done to the correct declaration in case we have nested
trait methods with the same name replaced.

Fixes #5504
esdrubal added a commit that referenced this issue Feb 2, 2024
@esdrubal
Fixes ReplaceDecls to no longer depend on filter hack.
0321952 
`ReplaceDecls` was using a hack to replace some function refs based on
the self function parameter, this didn't work for associated functions.

Now DeclMapping sources also contain a typeid that guarantees that the
replacement is done to the correct declaration in case we have nested
trait methods with the same name replaced.

Fixes #5504
IGI-111 pushed a commit that referenced this issue Feb 5, 2024
@esdrubal @JoshuaBatty
Fixes ReplaceDecls to no longer depend on filter hack. (#5541)
491a07f 
`ReplaceDecls` was using a hack to replace some function refs based on
the self function parameter, this didn't work for associated functions.

Now DeclMapping sources also contain a typeid that guarantees that the
replacement is done to the correct declaration in case we have nested
trait methods with the same name replaced.

Fixes #5504

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [x] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [x] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.

Co-authored-by: Joshua Batty <joshpbatty@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@esdrubal esdrubal

@vaivaswatha vaivaswatha

Labels
P: critical Should be looked at before anything else
Projects
None yet
Milestone
No milestone
4 participants
@xunilrj @esdrubal @vaivaswatha @IGI-111