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

Rust tidy check for Cargo.lock #33209

Closed
wants to merge 127 commits into from
Closed

Conversation

gsquire
Copy link
Contributor

@gsquire gsquire commented Apr 26, 2016

This is my attempt at implementing the check mentioned in #32901

@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 @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

println!("\n\tcargo update --manifest-path {:?}/Cargo.lock -p rustc", path);
*bad = true;
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should return if it is a success too so I don't run the command a bunch of times right?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah either way is fine by me, if we ask you to update multiple files that also seems fine!

@gsquire
Copy link
Contributor Author

gsquire commented Apr 26, 2016

@alexcrichton Thanks for the comments. I was able to simplify the check a lot after I thought about it some more.

@alexcrichton
Copy link
Member

@bors: r+ 3efc684ae12ba1d751cda2140d1c0889aca87fc4

Thanks!

@bors
Copy link
Contributor

bors commented Apr 27, 2016

⌛ Testing commit 3efc684 with merge 8131bbd...

@bors
Copy link
Contributor

bors commented Apr 27, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@gsquire
Copy link
Contributor Author

gsquire commented Apr 27, 2016

fatal: Invalid object name 'C'.

Well I have to say that I am not a Windows guy so I don't know what that error means.

@alexcrichton
Copy link
Member

Oh git there probably is interpreting it as something like <rev1>:<rev2>, perhaps the path name queried should be a relative path?

Refactor `ty::Visibility` methods to use a new trait `NodeIdTree` instead of the ast map.
@gsquire
Copy link
Contributor Author

gsquire commented Apr 27, 2016

I see. Would you like me to squash my commits by the way (if this commit passes)?

@alexcrichton
Copy link
Member

Sure!

Also just to confirm, this prints out an error locally if Cargo.toml changes?

@gsquire
Copy link
Contributor Author

gsquire commented Apr 27, 2016

If the return code isn't a success then it will print out that message, yes. I updated it to be more descriptive too but can change it if you still think it's unclear :)

@alexcrichton
Copy link
Member

Oh nah yeah the message is fine, just wanna confirm it passed a smoke test. I'll r+ with a squash

nrc and others added 18 commits May 3, 2016 10:51
Note that this whole PR is a [breaking-change] for clients of the Compiler API.
Overhaul borrowck error messages and compiler error formatting generally

This is a major overhaul of how the compiler reports errors. The primary goal is to be able to give many spans within the same overall context, such as this:

```
./borrow-errors.rs:73:17: 73:20: error: cannot borrow `*vec` as immutable because previous closure requires unique access [E0501]
70     let append = |e| {
                    ~~~ closure construction occurs here
71         vec.push(e)
           ~~~ previous borrow occurs due to use of `vec` in closure
72     };
73     let data = &vec[3];
                   ~~~ borrow occurs here
74 }
   ~ borrow from closure ends here
```

However, in the process we made a number of other changes:

- Removed the repetitive filenames from snippets and just give the line number.
- Color the line numbers blue so they "fade away"
- Remove the file name and line number from the error code suggestions since they don't seem to fit anymore. (This should probably happen in more places, like existing notes.)
- Newlines in between errors to help group them better.

This PR is not quite ready to land, but we thought it made sense to stop here and get some feedback from people at large. It'd be great if people can check out the branch and play with it. We'd be especially interested in hearing about cases that don't look good with the new formatting (I suspect they exist).

Here is a checklist of some pending work items for this PR. Some of them may be best left for follow-up PRs:

- [x] Accommodate multiple files in a `MultiSpan` (this should be easy)
  - In this case, we want to print filenames though.
- [x] Remove duplicate E0500 code.
- [x] Make the header message bold, rather than current hack that makes all errors/warnings bold
- [x] Update warning text color (yellow is hard to read w/ a white background)

Moved numerous follow-ups to: rust-lang#33240

Joint work with @jonathandturner.

Fixes rust-lang#3533
Overhaul borrowck error messages and compiler error formatting generally

This is a major overhaul of how the compiler reports errors. The primary goal is to be able to give many spans within the same overall context, such as this:

```
./borrow-errors.rs:73:17: 73:20: error: cannot borrow `*vec` as immutable because previous closure requires unique access [E0501]
70     let append = |e| {
                    ~~~ closure construction occurs here
71         vec.push(e)
           ~~~ previous borrow occurs due to use of `vec` in closure
72     };
73     let data = &vec[3];
                   ~~~ borrow occurs here
74 }
   ~ borrow from closure ends here
```

However, in the process we made a number of other changes:

- Removed the repetitive filenames from snippets and just give the line number.
- Color the line numbers blue so they "fade away"
- Remove the file name and line number from the error code suggestions since they don't seem to fit anymore. (This should probably happen in more places, like existing notes.)
- Newlines in between errors to help group them better.

This PR is not quite ready to land, but we thought it made sense to stop here and get some feedback from people at large. It'd be great if people can check out the branch and play with it. We'd be especially interested in hearing about cases that don't look good with the new formatting (I suspect they exist).

Here is a checklist of some pending work items for this PR. Some of them may be best left for follow-up PRs:

- [x] Accommodate multiple files in a `MultiSpan` (this should be easy)
  - In this case, we want to print filenames though.
- [x] Remove duplicate E0500 code.
- [x] Make the header message bold, rather than current hack that makes all errors/warnings bold
- [x] Update warning text color (yellow is hard to read w/ a white background)

Moved numerous follow-ups to: rust-lang#33240

Joint work with @jonathandturner.

Fixes rust-lang#3533
Make libsyntax::print::pp more idiomatic

Minor cleanup, and using VecDeque as a ring buffer instead of a vector.
diagnostics for E0432: imports are relative to crate root

This is curiously missing from both the short message and this long diagnostic.

Refs rust-lang#31573 (not sure if it should be considered "fixed" as the short message still only refers to extern crates).
match check: note "catchall" patterns in unreachable error

Caught as catchall patterns are:

* unconditional name bindings
* references to them
* tuple bindings with catchall elements

Fixes rust-lang#31221.
…Gomez

E0269: add suggestion to check for trailing semicolons

In situations where the value of the last expression must be inferred,
rustc will not emit the "you might need to remove the semicolon" warning,
so at least note this in the extended description.

Fixes: rust-lang#30497
typeck: remove confusing suggestion for calling a fn type

* It is not clear what a "base function" is.
* The suggestion just adds parens, so suggests calling without args.

The second point could be fixed with e.g. `(...)` instead of `()`,
but the preceding "note: X is a function, perhaps you wish to call it"
should already be clear enough.

Fixes: rust-lang#31341
…ichaelwoerister

rustc: Handle concurrent `create_dir` requests

The compiler created a directory as part of `-Z incremental` but that may be
hierarchically used concurrently so we need to protect ourselves against that.
lexer: do not display char confusingly in error message

Current code leads to messages like `... use a \xHH escape: \u{e4}` which is confusing.

The printed span already points to the offending character, which should be enough to identify the non-ASCII problem.

Fixes: rust-lang#29088
docs: Changed docs for `size_of` to describe size as a stride offset

Current documentation for `std::mem::size_of` is ambiguous, and the documentation for `std::intrinsics::size_of` incorrectly defines size.

This fix re-defines size as the offset in bytes between successive instances of a type, as described in LLVM's [getTypeAllocSize](http://llvm.org/docs/doxygen/html/classllvm_1_1DataLayout.html#a1d6fcc02e91ba24510aba42660c90e29).

Fixes: rust-lang#33266
libstd: correct the link to functions in io module documentation

Currently the link refers to it's own section of the documentation rather than the list of functions generated by rustdoc.

i.e. [this](http://doc.rust-lang.org/std/io/index.html#functions) should link to [this](http://doc.rust-lang.org/std/io/index.html#functions-1)
…back

In case we cannot produce a span for the location of the definition,
just do not show a span at all.

cc: rust-lang#29121
Refactor pretty printing to use the compiler API
typeck: when suggesting associated fns, do not show call site as fallback

In case we cannot produce a span for the location of the definition, just do not show a span at all.

cc: rust-lang#29121
@tamird
Copy link
Contributor

tamird commented May 3, 2016

Seems maybe worthwhile to print the git diff in the case where the check fails.

@alexcrichton
Copy link
Member

@gsquire do you wanna update this to include the updated Cargo.lock so this can land?

@gsquire
Copy link
Contributor Author

gsquire commented May 4, 2016

@alexcrichton Do I need to rebase onto master?

@alexcrichton
Copy link
Member

Probably, yeah, and then you can basically just run the command to update it and then commit those chanegs

@gsquire
Copy link
Contributor Author

gsquire commented May 4, 2016

Abandoning in favor of #33404 per my bad git skills. @alexcrichton

@gsquire gsquire closed this May 4, 2016
@gsquire gsquire deleted the cargo-lock-check branch May 4, 2016 17:30
@gsquire
Copy link
Contributor Author

gsquire commented May 4, 2016

@tamird I am sorry I overlooked your comment. I don't think git diff would have highlighted those changes. From my understanding, git diff-index will show the difference between HEAD and the current working tree for the path it may receive as an argument.

@tamird
Copy link
Contributor

tamird commented May 4, 2016

Maybe, but you're not actually printing the output of git diff-index - you're only inspecting the status code.

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.