Add completion for struct impls in submodules

[ReeceStevens]

Racer does not offer completion for struct impls if they are defined in submodules. This presents problems when working with generated libraries such as those built by svd2rust.

Relevant test case:

#[test]
fn completes_super_impl_fn() {
    let _lock = sync!();

    let src = r#"
    pub struct A { }

    pub mod a {
        impl super::A {
            pub fn return_number(&self) -> u8 {
                42
            }
        }

    }

    fn main() {
        let some_struct = A { };
        some_struct.~return_number()
    }
    "#;

    within_test_project(|| {
        let all = get_all_completions(src, None);
        println!("{:?}", all);
        let got = get_one_completion(src, None);
        assert_eq!("return_number", got.matchstr);
    })
}

I'm working on developing a solution for this, but I'm a fairly new Rustacean and could use a bit of direction if somebody has an idea of how to implement it. Feedback would be appreciated!

[TedDriggs]

I strongly suspect racer never even looks in submodules, since doing so could delay our time-to-results. If you run RUST_LOG=racer=trace racer complete ... you should be able to watch it travel through your code as it locates the struct, searches for impls in the current module, then returns.

This isn't a pattern I've encountered before in the wild. Could you post a gist or something showing the code generated by svd2rust?

P.S. Thanks so much for providing a minimal test case that demonstrates the issue. Makes the maintainers' lives 1000x easier :)

[ReeceStevens]

Hey @TedDriggs, thanks for the speedy response (sorry I didn't reciprocate that favor!). The debug log for the completion:

[reece@archtop src]$ RUST_LOG=racer=trace racer complete 17 24 main.rs
PREFIX 243,243,
DEBUG:racer::core: Field: contextstr is |some_struct|, searchstr is ||
DEBUG:racer::ast: visit_expr expr(4294967295: some_struct)
DEBUG:racer::ast: expr is a path P[some_struct]
DEBUG:racer::ast: resolve_ast_path P[some_struct]
DEBUG:racer::nameres: resolve_path_with_str P[some_struct]
DEBUG:racer::nameres: resolve_path P[some_struct] "main.rs" 243 ExactMatch
DEBUG:racer::nameres: resolve_name some_struct "main.rs" 243 ExactMatch Both
DEBUG:racer::nameres: search_local_scopes PathSegment { name: "some_struct", types: [] } "main.rs" 243 ExactMatch Both
DEBUG:racer::nameres: searching scope Both start: 149 point: 243 'some_struct' "main.rs" ExactMatch local: true, session: Session { .. }
DEBUG:racer::matchers: match_pattern_let point is 189
DEBUG:racer::typeinf: get_type_of match Match ["some_struct", "main.rs", 189, true, Let, [], [] |let some_struct = A { field: 5 };|]
DEBUG:racer::typeinf: get_type_of_let_expr calling parse_let |let some_struct = A { field: 5 };|
DEBUG:racer::ast: init node is Struct(path(A), [Field { ident: Spanned { node: field#0, span: Span { lo: BytePos(22), hi: BytePos(28), expn_id: ExpnId(4294967295) } }, expr: expr(4294967295: 5), span: Span { lo: BytePos(22), hi: BytePos(30), expn_id: ExpnId(4294967295) }, is_shorthand: false }], None)
DEBUG:racer::ast: visit_expr expr(4294967295: A{field: 5,})
DEBUG:racer::ast: find_type_match P[A], "main.rs"
DEBUG:racer::nameres: resolve_path_with_str P[A]
DEBUG:racer::nameres: resolve_path P[A] "main.rs" 189 ExactMatch
DEBUG:racer::nameres: resolve_name A "main.rs" 189 ExactMatch Type
DEBUG:racer::nameres: search_local_scopes PathSegment { name: "A", types: [] } "main.rs" 189 ExactMatch Type
DEBUG:racer::nameres: searching scope Type start: 149 point: 189 'A' "main.rs" ExactMatch local: true, session: Session { .. }
TRACE:racer::nameres: Closure definition match is looking for `A` in 115 characters
DEBUG:racer::nameres: search_scope found matches ExactMatch []
DEBUG:racer::nameres: search_scope_headers for |A| pt: 148
DEBUG:racer::nameres: search_scope_headers preblock is |fn main() |
DEBUG:racer::nameres: search_fn_args: found start of fn!! 138 |impl blah {fn main() {}}| A
DEBUG:racer::nameres: searching scope Type start: 0 point: 189 'A' "main.rs" ExactMatch local: true, session: Session { .. }
DEBUG:racer::matchers: found a struct |A|
DEBUG:racer::ast: LetTypeVisitor: ty is Some(Match(Match ["A", "main.rs", 11, true, Struct, [], [] |pub struct A|])). pos is 4, src is |let some_struct = A { field: 5 };|
DEBUG:racer::ast: destructure_pattern_to_ty point 4 ty Match(Match ["A", "main.rs", 11, true, Struct, [], [] |pub struct A|])    ||||||||    pat: pat(4294967295: some_struct)
DEBUG:racer::ast: destructure_pattern_to_ty matched an ident!
DEBUG:racer::core: complete_from_file context is Some(Match(Match ["A", "main.rs", 11, true, Struct, [], [] |pub struct A|]))
DEBUG:racer::nameres: got a struct, looking for fields and impl methods!! A
DEBUG:racer::nameres: searching for impl methods |Match ["A", "main.rs", 11, true, Struct, [], [] |pub struct A|]| || "main.rs"
DEBUG:racer::nameres: search_for_impls 11, A, "main.rs"
MATCH field,2,4,main.rs,StructField,u8
END

As you suspected, it does seem that racer doesn't look in submodules-- I believe the last three DEBUG lines suggest that it is looking for impls and cannot find any. I'm not sure how scope is delineated in these debug statements but I'll definitely take a closer look at this part of the code and see if there's a way to control looking through submodules in a way that doesn't hobble performance. Hopefully I'll have a chance to dig into this more later this week/weekend.

A representative snippet from a library generated by svd2rust (I didn't abbreviate this snippet so you can see the context). This is a definition of a control register with read, modify, write, and reset methods defined on it:

    #[doc = "control register"]
    pub struct CR {
        register: VolatileCell<u32>,
    }
    #[doc = "control register"]
    pub mod cr {
        #[doc = r" Value read from the register"]
        pub struct R {
            bits: u32,
        }
        #[doc = r" Value to write to the register"]
        pub struct W {
            bits: u32,
        }
        impl super::CR {
            #[doc = r" Modifies the contents of the register"]
            #[inline(always)]
            pub fn modify<F>(&self, f: F)
            where
                for<'w> F: FnOnce(&R, &'w mut W) -> &'w mut W,
            {
                let bits = self.register.get();
                let r = R { bits: bits };
                let mut w = W { bits: bits };
                f(&r, &mut w);
                self.register.set(w.bits);
            }
            #[doc = r" Reads the contents of the register"]
            #[inline(always)]
            pub fn read(&self) -> R {
                R { bits: self.register.get() }
            }
            #[doc = r" Writes to the register"]
            #[inline(always)]
            pub fn write<F>(&self, f: F)
            where
                F: FnOnce(&mut W) -> &mut W,
            {
                let mut w = W::reset_value();
                f(&mut w);
                self.register.set(w.bits);
            }
            #[doc = r" Writes the reset value to the register"]
            #[inline(always)]
            pub fn reset(&self) {
                self.write(|w| w)
            }
        }
    }

At first blush, it seems that the naive implementation I'm thinking of would be to limit searching submodules to one layer (since I don't believe super::super is valid or at least commonly used) and evaluate the performance hit. Does that seem like a valid path forward to you?

PS-- I realize "impl super::super::somestruct isn't commonly used" is a little ironic since impl super::somestruct isn't very common either haha

[ReeceStevens]

Also, glad I can help with the test case. It always helps me to clarify what the problem I want to solve is!

[TedDriggs]

After doing some digging on this front, I'm not sure racer's in a good spot to address this on our side, but the generated svd2rust code could be made more idiomatic and - as a side-effect -racer would work as-expected.

The change would be moving CR to cr::CR, with a pub use statement where CR used to be declared.

#[doc(inline)]
pub use cr::CR;

#[doc = "control register"]
pub mod cr {
    #[doc = "control register"]
    pub struct CR {
        register: VolatileCell<u32>,
    }

    #[doc = r" Value read from the register"]
    pub struct R {
        bits: u32,
    }
    #[doc = r" Value to write to the register"]
    pub struct W {
        bits: u32,
    }
    impl super::CR {
        #[doc = r" Modifies the contents of the register"]
        #[inline(always)]
        pub fn modify<F>(&self, f: F)
        where
            for<'w> F: FnOnce(&R, &'w mut W) -> &'w mut W,
        {
            let bits = self.register.get();
            let r = R { bits: bits };
            let mut w = W { bits: bits };
            f(&r, &mut w);
            self.register.set(w.bits);
        }
        #[doc = r" Reads the contents of the register"]
        #[inline(always)]
        pub fn read(&self) -> R {
            R { bits: self.register.get() }
        }
        #[doc = r" Writes to the register"]
        #[inline(always)]
        pub fn write<F>(&self, f: F)
        where
            F: FnOnce(&mut W) -> &mut W,
        {
            let mut w = W::reset_value();
            f(&mut w);
            self.register.set(w.bits);
        }
        #[doc = r" Writes the reset value to the register"]
        #[inline(always)]
        pub fn reset(&self) {
            self.write(|w| w)
        }
    }
}

This is how the std and core library expose some of their types:

• Result lives in core::result::Result but has a pub use in the prelude
• HashMap lives in std::collections::hash_map::HashMap but has a pub use statement in std::collections

Concerns with Proposed Fix

After doing some experiments, it looks like an impl block can appear anywhere in the declaring crate: See this gist for an example. I'm not aware of any libraries besides svd2rust that use impl super:: in any capacity, so I don't if adding one layer of lookup would address many cases or be a narrow fix just for this library.

If there's a fast way of resolving this that doesn't have far-reaching architectural implications then my opinion may change, but at the moment I'm leaning towards waiting for RLS to handle this.

[ReeceStevens]

@TedDriggs that makes a lot of sense. Totally understand the reluctance to making architectural changes because of an edge case-- seems like the better option here is to open a PR on svd2rust and try to use the idiom you suggested above (thanks for the example and refs to the standard library btw, that made it very clear).

Thanks very much for taking the time to dig through this and give me very useful feedback, I appreciate it. Keep up the great work :) Feel free to close out this issue and the associated PR if you'd like.

[TedDriggs]

Actually, the discussion on #rust on irc.mozilla.org right now is more split on this subject than I'd realized. I'd definitely suggest the PR on svd2rust, but I may need to amend my feelings on a racer PR here as well.