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.

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

Racer autocomplete #66

Merged
merged 3 commits into from
Dec 13, 2014

Conversation

andersforsgren
Copy link
Contributor

fixes #6

Known limitations:

  • Racer doesn't provide completions without a prefix. In VS you can hit ctrl+space on any (empty) line to get completions for local vars, keywords etc.
  • Keyword completion: included those as C# does that, but unhappy with the criteria for when they show up. Currently they are always included, but in places where only identifiers would be valid such as after a dot, keywords make no sense. The VS autocomplete handles this properly, but I'm unsure how to go about that.
  • Zero tests. I think I could mock the VS extension things but haven't.

Open questions:

  • Racer requires an environment var to point to the rust sources. In this PR there is no warning if this isn't set, there probably should be some way of setting this up, or a warning if it isn't.
  • Deployment of racer: currently binary is included (as a file) in project and included in msi/vsix. Not sure if this is the right thing to do (does llicense allow this? Where should it be documented what revision of racer is shipped etc.).
  • Autocomplete triggering: Currently mimics VS/C# autocomplete which is the most eager version completing on most keystrokes. One could imagine being less aggressive (especially if it was configurable)
  • Glyphs/icons for the completion items: didn't include any new icons, instead tried to make a mapping to the icons from VS/C#. Not all things map 1-1 of course.
  • Error handling and timeouts/performance. It currently calls synchronously into racer with a timeout (set att 3 sec). One could imagine instead doing it async anc cancelling on the next keystroke. I haven't pushed performance e.g. by trying huge files. If racer crashes, it should fail silently in release mode and just not display the completions.
  • Keep the keyword autocompletion?


if (!Enum.TryParse(langElemText, out elType))
{
DebugWrite("Failed to parse language element found in racer autocomplete response: {0}", elType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be DebugWrite("Failed to parse language element found in racer autocomplete response: {0}", langElemText)

@vosen
Copy link
Member

vosen commented Dec 9, 2014

First, bugs and weirdness

  • The racer binary is 64 bit and you didn't include .dll it depends on. Get us
    32bit version and its dependencies (you can check with dependency walker or included with Visual Studio
    dumpbin /imports).
  • You don't handle statics (try completing std::async::).
  • Why appending ( to functions? Unmodified VS doesn't do it.

Fix those three things and we can merge.

As for the other things. When it comes to mentioned limitations we can live with them (for now at least).

Open questions:

  • For racer we should use racer.exe from %PATH%, if it's not there fall back to
    one we ship. The proper way would be to build racer as part of Visual Rust
    (add racer repo as submodule) and create .rsproj to build it. This is a
    post-0.1 thing.
  • For env path if it doesn't exist, is empty or doesn't look like rust path we
    should error out by simply showing a message in the autocompletion popup.
    "There was an error blah blah. Please see Output view for the details".
    And show it only on explicit autocompletion.
  • Current autocompletion settings feel good to me. We can tweak them with some
    use and feedback.
  • Don't worry about icons, we can fix them later.
  • Current performance is sluggish, but we should measure it before coming to
    conclusions (What exactly is slow here? Text manipulations?
    Launching racer.exe? Something else?)
  • I'm for keeping the keyword autocompletion. Even if all of them are short, it
    feels good to know that IDE is eager to help you.

We can convert open questions issues and tackle them one at time.

@andersforsgren
Copy link
Contributor Author

Thanks for the feedback

Regarding the binary: completely forgot my rustc compiled native x64 by default, will make it x86. The dependencies of racer.exe (x86) are: the gcc dll (libgcc_s_dw2-1.dll) and the rust libraries. I'm unsure which of these need to be deployed? We can assume the end user has the rust libs, but what version? Racer currently depends on a nightly to build. If this turns out to be a can of worms, perhaps better to just stop deploying for now and rely on one in %PATH% instead? At least until racer doesn't depend on a nightly rust, and rust can do a better job with static linking under windows?

@vosen
Copy link
Member

vosen commented Dec 10, 2014

Fundamental problem here is that the user might have three, different, mutually incompatible versions of racer, Rust sources and rustc.
Now that I think of it, a better solution would be to run some sanity checks at package startup.

  • Check if rustc is in %PATH% (or in our settings). If it is not present, offer to download and run nightly installer.
  • Check if racer.exe is in %PATH% (or in our settings). If it is present, sanity check it by starting racer.exe. If it's not present fall back to racer we ship ourselves (with libgcc_s_dw2-1.dll and rust libraries). If our racer crashes suggest that it might be incompatible with rustc and tell user to compile it himself.
    AFAIK linking to rust libraries is ABI compatible. So if you link to syntax-4e7c5e5c.dll, next time binary incompatible libsyntax is compiled it'll be named syntax-<some_other_hash>.dll. If we want to make sure, we can always check with dumpbin.
  • Check if there's env var (or in our settings) pointing to something that looks like rust source. If we have nothing like that, offer to the user to git clone rust sources (we can get commit hash from rustc --version verbose).

The end goal is to have proper autocompletion with minimal fiddling (cloning, compiling and deploying racer.exe is quite annoying and might leave some users simply saying "fuck it" and be left with no autocompletion).

All in all, I think shipping our own version of racer.exe is something we should do.

@andersforsgren
Copy link
Contributor Author

Ok my suggestion for the scope of the issue/PR:
Bundle a "full" racer.exe (including gcc lib and 7 rust libs from version 4e7c5e5c). Do some santity checks at startup. Currently package startup doesn't seem to be active(?) so I'll make it at the completion command handler startup. This tries to find a racer exe in the path, if it doesn't exist it uses the bundled exe. It also looks for the RUST_SRC_PATH env var, and if that is missing/invalid it notifies the user of that.

The remaining bullets can be new issues I hope?

@vosen
Copy link
Member

vosen commented Dec 10, 2014

Just bundle "full" racer for now. I don't want this PR to unnecessarily grow in scope.
Implementing proper sanity checking and env lookup should be done hand in hand with the UI. Just using bundled racer.exe is OK for this PR.
Unless you really want to do it in this PR, in which case, sure go ahead, I'm not stopping you.

I'm not sure what you mean by package startup being inactive, but the package (VisualRustPackage) is lazy loaded the first time Rust package is needed (which should be at the first load of a Rust project).

And yes, remaining issues should be new issues.

@andersforsgren
Copy link
Contributor Author

Ah I was testing autocomplete with just starting devenv on a .rs file without rsproj which means the autocomplete command is loaded (by content type) but not the package! Must be careful then to not do mandatory init in package, to avoid crashing if a rust file is opened outside rsproj.
Ok I'll see how much sanity checking can fit nicely in scope and leave the rest.

@vosen
Copy link
Member

vosen commented Dec 10, 2014

Interesting, I though in such case VS would create some kind of implicit .rsproj.
In this case importing rust package with MEF should work.

- Changed racer.exe to 32bit
- Added racer dependencies
- Made a separate class to wrap the native racer executable & sanity
checks
- Moved racer exe + dependencies to subdir & made separate
wxs/componentgroup in msi
- Added sanity checks at startup for RUST_SRC_PATH and possibility to
load racer.exe from path
- removed auto-added bracket after Function name completion
- Fixed missing Static completions
- Fix wrong var used in log message when enum parsing failed
vosen added a commit that referenced this pull request Dec 13, 2014
@vosen vosen merged commit 3a9c447 into PistonDevelopers:master Dec 13, 2014
@vosen
Copy link
Member

vosen commented Dec 13, 2014

I've opened some follow-up issues. If I missed something feel free to open new ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Racer-based statement completion
2 participants