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

Fix Msbuild, fix project/item templates, improve intellisense. #286

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

MaulingMonkey
Copy link
Collaborator

@MaulingMonkey MaulingMonkey commented Jul 1, 2017

Unofficial Preview Builds

Overview of Changes

  • Fixed project/item templates (bad paths, missing from release .vsix)
  • Msbuild fixes (null derefs from non-messages in cargo 0.19, macro spans with invalid paths)
  • Intellisense improvements (using the optional 'substitute_file' for better results, refined when intellisense is triggered, added display of doc comments)
  • Misc. cleanup (racer and lexer refactoring, CompositionException fix, build vsix in debug)

Notes

  • It might be more appropriate to check if RustcMessageJson is null at the call site instead of inside the function
  • It might be worth logging features as reported by cargo somewhere
  • I've only tested the 2015 builds/paths
  • I'm a single-line conditional using heathen

Let me know if you'd prefer I do anything differently

…tead of crashing on non-messages.

This fixes a crash for me on cargo 0.19.0 (28d1d60d4 2017-05-16), example output follows:

I:\home\projects\test\testrust>cargo build --message-format=json 2>NUL
{"features":["default","use_std"],"filenames":["I:\\home\\projects\\test\\testrust\\target\\debug\\deps\\liblibc-f4ccf2fa58092d85.rlib"],"fresh":true,"package_id":"libc 0.2.24 (registry+https://github.com/rust-lang/crates.io-index)","profile":{"debug_assertions":true,"debuginfo":2,"opt_level":"0","overflow_checks":true,"test":false},"reason":"compiler-artifact","target":{"crate_types":["lib"],"kind":["lib"],"name":"libc","src_path":"C:\\Users\\MikeR\\.cargo\\registry\\src\\github.com-1ecc6299db9ec823\\libc-0.2.24\\src\\lib.rs"}}
{"features":[],"filenames":["I:\\home\\projects\\test\\testrust\\target\\debug\\deps\\librand-76456d761c2cb637.rlib"],"fresh":true,"package_id":"rand 0.3.15 (registry+https://github.com/rust-lang/crates.io-index)","profile":{"debug_assertions":true,"debuginfo":2,"opt_level":"0","overflow_checks":true,"test":false},"reason":"compiler-artifact","target":{"crate_types":["lib"],"kind":["lib"],"name":"rand","src_path":"C:\\Users\\MikeR\\.cargo\\registry\\src\\github.com-1ecc6299db9ec823\\rand-0.3.15\\src\\lib.rs"}}
{"message":{"children":[],"code":{"code":"E0425","explanation":"\nAn unresolved name was used.\n\nErroneous code examples:\n\n```compile_fail,E0425\nsomething_that_doesnt_exist::foo;\n// error: unresolved name `something_that_doesnt_exist::foo`\n\n// or:\n\ntrait Foo {\n    fn bar() {\n        Self; // error: unresolved name `Self`\n    }\n}\n\n// or:\n\nlet x = unknown_variable;  // error: unresolved name `unknown_variable`\n```\n\nPlease verify that the name wasn't misspelled and ensure that the\nidentifier being referred to is valid for the given situation. Example:\n\n```\nenum something_that_does_exist {\n    Foo,\n}\n```\n\nOr:\n\n```\nmod something_that_does_exist {\n    pub static foo : i32 = 0i32;\n}\n\nsomething_that_does_exist::foo; // ok!\n```\n\nOr:\n\n```\nlet unknown_variable = 12u32;\nlet x = unknown_variable; // ok!\n```\n\nIf the item is not defined in the current module, it must be imported using a\n`use` statement, like so:\n\n```ignore\nuse foo::bar;\nbar();\n```\n\nIf the item you are importing is not defined in some super-module of the\ncurrent module, then it must also be declared as public (e.g., `pub fn`).\n"},"level":"error","message":"cannot find value `guess_no2` in this scope","rendered":null,"spans":[{"byte_end":604,"byte_start":595,"column_end":23,"column_start":14,"expansion":null,"file_name":"src\\main.rs","is_primary":true,"label":"did you mean `guess_no`?","line_end":24,"line_start":24,"suggested_replacement":null,"text":[{"highlight_end":23,"highlight_start":14,"text":"\t\t\t\t\tif      guess_no2 < target { println!(\"Guess too low\"); }"}]}]},"package_id":"testrust 0.1.0 (path+file:///I:/home/projects/test/testrust)","reason":"compiler-message","target":{"crate_types":["bin"],"kind":["bin"],"name":"testrust","src_path":"I:\\home\\projects\\test\\testrust\\src\\main.rs"}}
{"message":{"children":[],"code":null,"level":"error","message":"aborting due to previous error","rendered":null,"spans":[]},"package_id":"testrust 0.1.0 (path+file:///I:/home/projects/test/testrust)","reason":"compiler-message","target":{"crate_types":["bin"],"kind":["bin"],"name":"testrust","src_path":"I:\\home\\projects\\test\\testrust\\src\\main.rs"}}
I've observed that in practice this allows the completion of basic static methods, such as "rand::thread_rng().gen_range(1,11);"
… directory.

This was presumably originally done to improve "racer find-definition"'s results.
However, it takes an optional [substitute_file] parameter we can use instead:
    racer find-definition <linenum> <charnum> <path> [substitute_file]

This change eliminates the following churn in the "Visual Rust" output tab:
    Apply change: File created: I:\home\projects\test\testrust\src\main.rs~RFa2df0b.TMP
    Apply change: File created: I:\home\projects\test\testrust\src\main.rs~RFa2df0b.TMP
    Failed to apply change 'File created: I:\home\projects\test\testrust\src\main.rs~RFa2df0b.TMP':System.ComponentModel.Win32Exception (0x80004005): Access is denied
       at VisualRust.ProjectSystem.FileSystem.ToShortPath(String path)
       at ...
    Apply recovery change: Directory created: I:\home\projects\test\testrust\
Errors in macro expansions sometimes don't have real "file_name"s at every expansion level.
This simply attempts to recursively expand until we can combine the paths sanely.
Other alternatives might include comparing against target src_path, or looking for an ".rs" extension.
- Will make parsing available to future "signature help" popup hints.
- Handle unescaping of rust strings, semicolon escapes, etc.
- Document the heck out of members, complete with examples.
User Interface changes:
- F1 Help can now look up types, primitives.
- Find definition now fully delegates to 'racer find-defition'.
- Rust completions no longer triggered when definining new identifiers.

Tokenization changes:
- Replaced misused/understood "GetTokensAtPosition" with "GetLineTokensUpTo" and "GetTokensAt".
- Documented and added explicit examples to help head off any confusion.
- Added tracing of RustCompletionCommandHandler to see impacts.

Fixed problems / rationale:
- Single character tokens could previously never be "currentToken".
- "currentToken" was usually null.
- "leftToken" would often be the *last* token when typing in the *middle* of a line.

Possible future paths:
- Some kind of multiline token fetcher that fetches enough tokens for us to work with, without tokenizing the entire document.
- Full blown AST?
@Boddlnagg Boddlnagg requested a review from vosen July 6, 2017 09:17
Previously, all documentation for an identifier was displayed.  This
included "# Examples" sections and lots of potentially long and rambling
documentation.

Rustdoc conventions suggest an initial "summary line", which would be
a good defacto standard to display.  In practice, the initial few
paragraphs before the first header section also seem worth displaying,
but that's perhaps more my personal taste.  I've provided accessors for
getting at "Plain" versions of both.

Refactoring:
- RustCompletionSource:  Move regexps to RacerMatch.
- RacerMatch:  Expose Plain{Documentation,SummaryLine,SummaryParagraphs}
Floating point numbers aren't considered STRUCTURAL, so this doesn't
incorrectly trigger for "12." and similar expressions.
@MaulingMonkey MaulingMonkey changed the title Fix cargo 0.19.0 JSON crash, build errors from project and item templates. Fix Msbuild, fix project/item templates, improve intellisense. Jul 12, 2017
The default errors that e.g. Visual Studio gives when failing to do
either of these are unhelpful at best, downright confusing at worst.
As I'm apparently not alone in overlooking these steps, they seem worth
documenting.

Ref: PistonDevelopers#294
     "Multiple Issues opening and building the solution in VS2017"
Also build Debug platforms for completeness.

Configurations:
  - Removed "Debug.Lab" and "Release.Lab" - these seem unnecessary.
  - Always build 'everything', instead of a weird hodge podge of missing
    projects for any given configuration.
  - Always build project configurations by the same name.

Platforms:
  - "Mixed Platforms" replaced with "All VS", which builds everything always.
  - New "VS2015 Only" builds everything but VS2017-specific projects.
  - New "VS2017 Only" builds everything but VS2015-specific projects.
  - "x86", "x64", and "Any CPU" are all removed.  We always want to build
    a mixture of "Any CPU" (most projects) and "x86" (setup projects.)

Closes PistonDevelopers#295
       "Clean up solution config/platform matrix."
.debug.vsixmanifest already referenced LICENSE.txt

Ref: PistonDevelopers#301
    "Could not uninstall VSIX - looking for LICENSE.txt"
@Boddlnagg
Copy link
Contributor

Maybe we should get some attention from the Rust team to the situation here (that VisualRust is essentially unmaintained, because @vosen does not respond ...).

@MaulingMonkey
Copy link
Collaborator Author

Mmm. Or people from the PistonDevelopers group? I don't know the hierarchy/dynamics here. Or I could try @vosen 's email?

It would be good to unblock contribution. I effectively stopped working on additional Visual Rust features because this pull request is already uncomfortably large. I want to avoid causing another dead fork (it looked like there was one adding AST parsing?) with duplicated or wasted effort.

Since this pull request, my free time has been curtailed by a job as well, although I'd be willing to try and help tackle maintainer duties if added to the repository. Although that doesn't get this code reviewed by someone else, which would still be nice to have - although maybe not at the expense of progress.

@Boddlnagg
Copy link
Contributor

Boddlnagg commented Sep 19, 2017

@MaulingMonkey I have added you as a collaborator to this repository. I will try to find some time to review your PR, if @vosen remains unresponsive ...

You could definitely try to reach him by email ...

@MaulingMonkey
Copy link
Collaborator Author

Finally pinged @vosen via email on Monday, no response so far though. I'm inclined to go ahead and just merge this next week sometime - I can always respond to feedback after the merge too.

@MaulingMonkey
Copy link
Collaborator Author

#302 may block merging this - I tried to manually make a new build via appveyor and hit the quota error again.

@Boddlnagg
Copy link
Contributor

Okay, from my side you can feel free to merge this whenever you know how to deal with #302 👍

@Boddlnagg
Copy link
Contributor

You might also be interested in https://github.com/dgriffen/rls-vs2017

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.

None yet

2 participants