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

Some improvements to the Vim Cargo compiler file. #17513

Closed
wants to merge 4 commits into from

Conversation

dradtke
Copy link

@dradtke dradtke commented Sep 24, 2014

Looks like I made my previous PR a little too hastily. =)

This PR fixes a couple issues that I discovered with my previous revision:

  1. Updated the errorformat to ignore "pointer lines" so that they don't show up in the output (with quickfix jumping, they're redundant and unnecessary).
  2. Renamed a couple variables to be more in line with Cargo's terminology (g:cargo_toml_name should now be g:cargo_manifest_name).
  3. Added support for errors reported with absolute paths (looks to be the case when compiling an executable instead of a library).
  4. Most importantly, added support for errors reported while compiling a dependency. When building a Cargo package with local dependencies, if one of those dependencies failed to compile, the quickfix would be completely broken as it assumed that all errors were relative to the local manifest, or the closest Cargo.toml. With this update, it now pays attention to lines that end with (file://<path>), and from then on adjusts all errors to be relative to <path>.

As a side note, that <path> output is somewhat broken on Windows. While file:///home/damien/... on *Nix is a valid URI, file:///C:/Users/damien/... on Windows is not, because C:/ (or whatever the drive is) should take the place of the third slash which is *Nix's root, not be appended to it. I added a workaround for this in my script, but I figured I'd mention it to see if this is a bug in how Rust formats paths.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

@chris-morgan
Copy link
Member

Have you examined the rustc compiler’s errorformat? None of the items in there is there by mistake, and I would expect that the cargo compiler should be using the same errorformat.

I also still say that rustc and cargo need to be altered so that they can produce absolute file names, which would simplify things further.

@dradtke
Copy link
Author

dradtke commented Oct 7, 2014

@chris-morgan, I haven't really looked at it, but that's a good idea. It seemed to work with the default errorformat so I just built it off that, but I can integrate the one from rustc.

It would make things a lot simpler if Cargo would always output absolute file names, but one thing that I like about the final result of this compiler is that the paths are relative, but correct. I personally think it's unnecessary extra noise to always prepend each file path with the absolute path of the project, so there are two possible ways to improve this:

  1. Have Cargo output absolute file names, either by default or with a switch, and use QuickFixCmdPost to make each path relative to the current directory.
  2. Have Cargo output file names relative to the current directory instead of to Cargo.toml, either by default or with a switch, rendering QuickFixCmdPost unnecessary.

I like the idea of Cargo providing switches to toggle between different absolute/relative file path modes, but otherwise would be happy with a sane default.

@alexcrichton
Copy link
Member

r? @kballard or @chris-morgan

@lilyball
Copy link
Contributor

The new settings need to be added to src/etc/vim/doc/rust.txt.

CompilerSet errorformat&
CompilerSet makeprg=cargo\ $*
if exists('g:cargo_makeprg_params')
execute 'CompilerSet makeprg=cargo\ '.g:cargo_makeprg_params.'\ $*'
Copy link
Contributor

Choose a reason for hiding this comment

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

g:cargo_makeprg_params will need escaping. If it contains any special characters (such as a space), they will behave badly otherwise. I don't know offhand if there's a canonical way to do this, but I expect that this would work: escape(g:cargo_makeprg_params, ' \|"').

@dradtke
Copy link
Author

dradtke commented Oct 24, 2014

Made a number of updates based on your comments. Thanks for the feedback!

@alexcrichton
Copy link
Member

re-r? @kballard

if s:toml_dir != ''
let s:local_manifest = findfile(s:cargo_manifest_name, '.;')
if s:local_manifest != ''
let s:local_manifest = fnamemodify(s:local_manifest, ':p:h').'/'
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a hard tab on this line.

@lilyball
Copy link
Contributor

lilyball commented Nov 3, 2014

Besides the above comments, the new g:cargo_manifest_name setting still needs to be added to src/etc/vim/doc/rust.txt.

@dradtke
Copy link
Author

dradtke commented Nov 4, 2014

I made some changes based on the feedback.

Looking at g:cargo_manifest_name, it occurred to me that it won't be at all useful for values outside of cargo.toml unless it gets passed in as a flag to Cargo. Most of the commands support a --manifest-path option, but since it's not universal (new for example doesn't support it), there's no easy way to do it since it would need to change makeprg based on the command being run.

I don't think this should block this PR, but it would be great if Cargo accepted --manifest-path as a direct flag, and simply ignored it for subcommands that don't need it. Then g:cargo_manifest_name could theoretically be anything and Cargo will still be able to find it, at least when it's invoked from Vim.

bors added a commit that referenced this pull request Nov 22, 2014
Looks like I made my previous PR a little too hastily. =)

This PR fixes a couple issues that I discovered with my previous revision:

1. Updated the errorformat to ignore "pointer lines" so that they don't show up in the output (with quickfix jumping, they're redundant and unnecessary).
2. Renamed a couple variables to be more in line with Cargo's terminology (`g:cargo_toml_name` should now be `g:cargo_manifest_name`).
3. Added support for errors reported with absolute paths (looks to be the case when compiling an executable instead of a library).
4. Most importantly, added support for errors reported while compiling a dependency. When building a Cargo package with local dependencies, if one of those dependencies failed to compile, the quickfix would be completely broken as it assumed that all errors were relative to the local manifest, or the closest Cargo.toml. With this update, it now pays attention to lines that end with `(file://<path>)`, and from then on adjusts all errors to be relative to `<path>`.

As a side note, that `<path>` output is somewhat broken on Windows. While `file:///home/damien/...` on *Nix is a valid URI, `file:///C:/Users/damien/...` on Windows is not, because `C:/` (or whatever the drive is) should take the place of the third slash which is *Nix's root, not be appended to it. I added a workaround for this in my script, but I figured I'd mention it to see if this is a bug in how Rust formats paths.
@bors bors closed this Nov 22, 2014
lnicola pushed a commit to lnicola/rust that referenced this pull request Jul 11, 2024
fix: completions after async kw

fix rust-lang#17500

### Changes

1. fix completions after async kw
2. fix completions for `async` kw in trait
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.

6 participants