Q: racer integration, find_definition usage

[jwilm]

I recently started integrating racer for YouCompleteMe support, and this is the first of what I'm sure will be many questions 😄. This question regards find_definition.

pub fn find_definition(src: &str, filepath: &path::Path, pos: usize, session: SessionRef) -> Option<Match>

One of the arguments is a source buffer. If I don’t write the buffer to disk first, I get a panic (full bt below).

thread 'engine::racer::tests::find_definition' panicked at 'index 0 and/or 89 in `` do not lie on character boundary', ../src/libcore/str/mod.rs:1441

However, writing the buffer to disk completely resolves that problem. It seems to me that, given a buffer, reading the initial file should be unnecessary. Is this a requirement? I would like to change the behavior to not do that if possible. I actually couldn’t find where it was doing that IO, either. Additionally, since this method is doing I/O, maybe it should return a Result instead of panicking while indexing a slice.

Backtrace

   6:        0x10c490097 - rust_begin_unwind
   7:        0x10c4b4060 - panicking::panic_fmt::h6c78ce0128588a957HK
   8:        0x10c4b7b60 - str::slice_error_fail::hae4b5de9c67475548OS
   9:        0x10c0f44b5 - str::traits::_<impl>::index::he8945502e3410953mBS
  10:        0x10c0f437e - string::_<impl>::index::h547be55ef8b5db17i3f
  11:        0x10c0f3b92 - core::_<impl>::deref::h6728862e5c44cb4atMa
  12:        0x10c1030aa - scopes::mask_comments::h758821d339eb152bJeb
  13:        0x10c10e321 - scopes::scope_start::h49adfd7fdda43b7544a
  14:        0x10c1c6f90 - nameres::search_local_scopes::h2aa638ee8218b5856xe
  15:        0x10c1bc129 - nameres::resolve_name::ha8fef8b2d1cc0115pPe
  16:        0x10c10912e - nameres::resolve_path::hd3f93ff20151730eeZe
  17:        0x10c10c842 - core::find_definition_::h8b9c5b4772ebed00tYa
  18:        0x10c10c16b - core::find_definition::h36205eee38345da54Xa
  19:        0x10c0dc75b - engine::racer::_<impl>::find_definition::h6f5d2878fe1b3851yea
  20:        0x10c0e34c0 - engine::racer::tests::find_definition::h607071473acdcea1yla
  21:        0x10c2469eb - boxed::_<impl>::call_box::call_box::h5504267803333294456
  22:        0x10c2490f0 - sys_common::unwind::try::try_fn::try_fn::h8503899523036047578
  23:        0x10c48fed8 - __rust_try
  24:        0x10c48de7e - sys_common::unwind::try::inner_try::h4c901f4c815f0df8y9r
  25:        0x10c2494ea - boxed::_<impl>::call_box::call_box::h16742266516396999763
  26:        0x10c4918ad - sys::thread::_<impl>::new::thread_start::h9bc185148c33bf72Kvw
  27:     0x7fff8add49b0 - _pthread_body
  28:     0x7fff8add492d - _pthread_start

[jwilm]

@birkenfeld: @phildawes mentioned you might have some thoughts on this.

[phildawes]

Hi @jwilm ,

The &str arg to find-definition is a bit historical and needs cleaning up, I didn't even realise it was still there!

Basically racer loads source from disk lazily and is expecting there to be a file there. Recently @birkenfield added a cache for source artifacts in the Session object.

The current racer plugins supply a filename and optionally a tmpfile to load the source from.

e.g. In the main.rs find_definition() function racer loads source from a tmpfile, using the session

    let session = core::Session::from_path(&fpath, &substitute_file);
    let src = session.load_file(&fpath);
    let pos = scopes::coords_to_point(&src, linenum, charnum);
    core::find_definition(&src, &fpath, pos, &session).map(|m| match_fn(m, &session));

I think the way to solve your problem is to load the source text into the session before calling find_definition. You'll probably want to add a new method to Session to load source text if that is how racerd is going to work.
Hope this helps, sorry this is a bit crufty.

[phildawes]

Btw, I agree re: Result. When racer was first written Result wasn't as big a part of rust as it is now, and there was no From trait conversion easing Error propagation. Even rustc's libsyntax paniced on syntax errors until I posted rust-lang#23857 in april.

Having said that, panic backtraces are really handy, especially if you compile with debugging to get line numbers.

[jwilm]

Good news, everyone!

I've made some serious progress on racerd and the YCM integration. Here's a quick completion demo.

Of course, this wouldn't be possible with all the work you've done here on racer; thank you!

There is one challenge I've run into that may not be a simple fix. One thing that I've done in racerd is keep a Session alive across http requests. Aside from saving tons of disk io, it measurably improved response time. This is great!. In my fork of racer, I've got some changes to the Session that allow me to update files in the FileCache without reading from disk. This is critical in YCM since each request contains all of the (potentially unsaved) buffers the user has open.

The problem here is that I cannot get a mutable reference to the IndexedSource object currently tracked in the FileCache's raw_map or masked_map. To get around this so far, I'm just asking the arena for a new object and updating the reference stored in those maps. Obviously this is really bad because the arena will just grow indefinitely.

Do you guys have any thoughts on how to make this work and keep the niceness of using an Arena in the FileCache? I was thinking the most straight forward way would be to stuff a RefCell into the IndexedSource. Aside from just making that work, I don't know what kind of consequences mutating the source String in IndexedSource would have. It seems that any Src objects it has handed out should become invalidated. Beyond that, I can't speculate about the consequences. @birkenfeld, I think you may have some insight here already since you wrote the latest Session implementation.

Thanks again!

[phildawes]

Hi @jwilm,

This looks awesome!
Personally I'd be inclined to ditch the Arena for now and just try to find a way to do this using vanilla heap allocation. If it causes performance problems then we can re-visit.
(other people may disagree).

[birkenfeld]

Well, the point of the Arena is to give all source references the lifetime of the session. Without it, you're relegated to using Rc in a lot of places, or having a crapton of lifetimes to care about.

In a long running process, I think you'll have to use a two-layered structure: an object (that we don't have currently) that is kept alive between requests, and an object (like the Session) that persists throughout one request.

One way I can think of right now is to have the arena append-or-invalidate-only during a request, and to clean it out after/before the next one, in the code that mutably owns the cache. This likely requires a specialized Arena like type that you'll have to write (not that difficult probably).

[jwilm]

@birkenfeld is the idea with the two-layered object is that we know no references are handed out once the Session is destroyed? That's the issue I keep coming back to with a mutable file cache in the current setup.

[birkenfeld]

Yep, that's the idea.

[jwilm]

@birkenfeld

I started looking into separating out the FileCache and Session as a proof-of-concept before going and making an Arena with recyclable members, and I ran into some unexpected lifetime errors.

Here are the changes. The Session::from_path method now accepts a &FileCache in addition to the two paths. So, I updated the tests to have a cache in the stack and pass it into all of the from_path calls. So, I get errors about the session reference not living long enough and suggesting to use a let bindings. Ok, done. More lifetime errors; they are pretty ambiguous now. They all look like so:

tests/system.rs:1446:49: 1446:56 error: `session` does not live long enough
tests/system.rs:1446     let got = find_definition(src, &path, pos, &session).unwrap();
                                                                     ^~~~~~~
tests/system.rs:1444:40: 1450:2 note: reference must be valid for the block suffix following statement 4 at 1444:39...
tests/system.rs:1444     let cache = core::FileCache::new();
tests/system.rs:1445     let session = core::Session::from_path(&cache, &path, &path);
tests/system.rs:1446     let got = find_definition(src, &path, pos, &session).unwrap();
tests/system.rs:1447     fs::remove_file(&path).unwrap();
tests/system.rs:1448     assert_eq!("a", got.matchstr);
tests/system.rs:1449     assert_eq!(9, got.point);
                     ...
tests/system.rs:1445:66: 1450:2 note: ...but borrowed value is only valid for the block suffix following statement 5 at 1445:65
tests/system.rs:1445     let session = core::Session::from_path(&cache, &path, &path);
tests/system.rs:1446     let got = find_definition(src, &path, pos, &session).unwrap();
tests/system.rs:1447     fs::remove_file(&path).unwrap();
tests/system.rs:1448     assert_eq!("a", got.matchstr);
tests/system.rs:1449     assert_eq!(9, got.point);

The lifetime issues are surprising in the first place since the find_definition doesn't return a reference to anything. Normally I would go through a little more trial and error to figure out the problem, but updating all of the tests for each change is cumbersome even with vim macros. I appreciate any time you can spare to look at this.

Thanks!

[birkenfeld]

The issue is that the SessionRef type is defined as &'s Session<'s>, so the session and the cache would have to have the same lifetime.

A (possible) fix is here: https://github.com/birkenfeld/racer/tree/wip-reusable-cache

(I would probably further rename the 's lifetime to 'c and 'r to 's.)

[birkenfeld]

Actually, once you untie these two lifetimes, there is no reason for SessionRef to exist anymore, and it can just be &Session in function arguments and &'s Session<'c> in struct definitions.

[jwilm]

ugh! I looked right as SessionRef and didn't think anything of it 😳

Thanks for helping with this @birkenfeld; you saved me a lot of time banging my head against the wall 😁.

[jwilm]

I have a prototype of reusable allocations implemented within the FileCache. When a cache entry is updated, a ref for the previous value is pushed onto a free list. When the Session object is dropped, it calls increment_generation on the FileCache, and the references in the free list are moved into an available list. In that process, the previous value is dropped.

This approach has a soundness issue; since the increment_generation is a public method if file cache, it is possible for it to be called while the Session is active and has a reference handed out for something in the free list.

The next step here is where I struggle a bit. We need an arena implementation which supports the free operation and whose API doesn't suffer from the same issue as above. I came across this proposal from a couple of years ago which suggests such an API. It relies on a smart pointer type instead of handing out raw references. Wouldn't that have a big impact on how arenas are used? Is that really any better than dealing with Rc<T>? This may just be a lack of thorough understand of the Deref trait on my part.

What do you guys think?

[jwilm]

Closing this since all of the discussions have been resolved. Will open new issues as necessary.

Thanks again for all the help.