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

Rollup of 15 pull requests #88824

Merged
merged 38 commits into from
Sep 11, 2021
Merged

Rollup of 15 pull requests #88824

merged 38 commits into from
Sep 11, 2021

Conversation

Manishearth
Copy link
Member

Successful merges:

Failed merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

m-ou-se and others added 30 commits June 9, 2021 12:43
It was accidentally changed to use `opts()` in rust-lang#86451.

I also renamed `opts()` to `main_body_opts()` to make this kind of
accidental change less likely.
This fixes odd renderings when these features are used in the first
paragraph of documentation for an item. This is an extension of rust-lang#87270.
Previous version wrongly used `but` while the two parts of the sentence are not contradicting but completing with each other.
this also renders them as `_`, which rustdoc previously did not.
Note that this incorrectly suggests a shared borrow,
but at least we know it's happening.
These were deleted in https://reviews.llvm.org/D108614, and in C++ I
definitely see the argument for their removal. I didn't try and
propagate the changes up into higher layers of rustc in this change
because my initial goal was to get rustc working against LLVM HEAD
promptly, but I'm happy to follow up with some refactoring to make the
API on the Rust side match the LLVM API more directly (though the way
the enum works in Rust makes the API less scary IMO).

r? @nagisa cc @nikic
Otherwise we're kind of reimplementing the inverse of the well-named
methods, and that's not a direction we want to go.
Turns out we can also use Attribute::get*() methods here, and avoid the
AttrBuilder and an extra helper method here.
This commit focuses on emitting clean errors for the following syntax
error:

```
Some(42).map(|a|
    dbg!(a);
    a
);
```

Previous implementation tried to recover after parsing the closure body
(the `dbg` expression) by replacing the next `;` with a `,`, which made
the next expression belong to the next function argument. As such, the
following errors were emitted (among others):
  - the semicolon token was not expected,
  - a is not in scope,
  - Option::map is supposed to take one argument, not two.

This commit allows us to gracefully handle this situation by adding
giving the parser the ability to remember when it has just parsed a
closure body inside a function call. When this happens, we can treat the
unexpected `;` specifically and try to parse as much statements as
possible in order to eat the whole block. When we can't parse statements
anymore, we generate a clean error indicating that the braces are
missing, and return an ExprKind::Err.
…akis

Ignore derived Clone and Debug implementations during dead code analysis

This pull request fixes rust-lang#84647. Derived implementations of `Clone` and `Debug` always trivially read all fields, so "field is never read" dead code warnings are never triggered. Arguably, though, a user most likely will only be interested in whether _their_ code ever reads those fields, which is the behavior I have implemented here.

Note that implementations of `Clone` and `Debug` are only ignored if they are `#[derive(...)]`d; a custom `impl Clone/Debug for ...` will still be analyzed normally (i.e. if a custom `Clone` implementation uses all fields of the struct, this will continue to suppress dead code warnings about unused fields); this seemed like the least intrusive change to me (although it would be easy to change — just drop the `&& [impl_]item.span.in_derive_expansion()` in the if conditions).

The only thing that I am slightly unsure about is that in rust-lang#84647, `@matklad` said
> Doesn't seem easy to fix though :(

However, it _was_ pretty straightforward to fix, so did I perhaps overlook something obvious? `@matklad,` could you weigh in on this?
…tolnay

Add proc_macro::Span::{before, after}.

This adds `proc_macro::Span::before()` and `proc_macro::Span::after()` to get a zero width span at the start or end of the span.

These are equivalent to rustc's `Span::shrink_to_lo()` and `Span::shrink_to_hi()` but with a less cryptic name. They are useful when generating diagnostlics like "missing \<thing\> after \<thing\>".

E.g.

```rust
syn::Error::new(ident.span().after(), "missing `:` after field name").into_compile_error()
```
Fix stray notes when the source code is not available

Fixes rust-lang#87060. To reproduce it with a local build of rustc, you have to copy the compiler (e.g. `build/x86_64-unknown-linux-gnu/stage1/`) somewhere and then rename the compiler source directory (maybe there is a smarter way as well). Then, rustc won't find the standard library sources and report stray notes such as
```
note: deref defined here
```
with no location for "here". Another example I've found is this:
```rust
use std::ops::Add;

fn foo<T: Add<Output=()>>(x: T) {
    x + x;
}

fn main() {}
```
```
error[E0382]: use of moved value: `x`
  --> binop.rs:4:9
   |
3  | fn foo<T: Add<Output=()>>(x: T) {
   |                           - move occurs because `x` has type `T`, which does not implement the `Copy` trait
4  |     x + x;
   |     ----^
   |     |   |
   |     |   value used here after move
   |     `x` moved due to usage in operator
   |
note: calling this operator moves the left-hand side
help: consider further restricting this bound
   |
3  | fn foo<T: Add<Output=()> + Copy>(x: T) {
   |                          ^^^^^^

error: aborting due to previous error
```
where, again, the note is supposed to point somewhere but doesn't. I have fixed this by checking whether the corresponding source code is actually available before emitting the note.
Emit suggestion when passing byte literal to format macro

Closes rust-lang#86865
…races, r=estebank

Emit proper errors when on missing closure braces

This commit focuses on emitting clean errors for the following syntax
error:

```
Some(42).map(|a|
    dbg!(a);
    a
);
```

Previous implementation tried to recover after parsing the closure body
(the `dbg` expression) by replacing the next `;` with a `,`, which made
the next expression belong to the next function argument. As such, the
following errors were emitted (among others):
  - the semicolon token was not expected,
  - a is not in scope,
  - Option::map is supposed to take one argument, not two.

This commit allows us to gracefully handle this situation by adding
giving the parser the ability to remember when it has just parsed a
closure body inside a function call. When this happens, we can treat the
unexpected `;` specifically and try to parse as much statements as
possible in order to eat the whole block. When we can't parse statements
anymore, we generate a clean error indicating that the braces are
missing, and return an ExprKind::Err.

Closes rust-lang#88065.

r? `@estebank`
…erence-to-for-loop-iter, r=nagisa

fix(rustc): suggest `items` be borrowed in `for i in items[x..]`

Fixes rust-lang#87994
Fix issues with Markdown summary options

- Use `summary_opts()` for Markdown summaries
- Enable all main body Markdown options for summaries
…lds-count, r=Manishearth

Rustdoc coverage fields count

Follow-up of rust-lang#88688.

Instead of requiring enum tuple variant fields and tuple struct fields to be documented, we count them if they are documented, otherwise we don't include them in the count.

r? `@Manishearth`
RustWrapper: avoid deleted unclear attribute methods

These were deleted in https://reviews.llvm.org/D108614, and in C++ I
definitely see the argument for their removal. I didn't try and
propagate the changes up into higher layers of rustc in this change
because my initial goal was to get rustc working against LLVM HEAD
promptly, but I'm happy to follow up with some refactoring to make the
API on the Rust side match the LLVM API more directly (though the way
the enum works in Rust makes the API less scary IMO).

r? ``@nagisa`` cc ``@nikic``
…ks, r=nbdd0121

Fix table in docblocks

"Overwrite" of rust-lang#88702.

Instead of adding a z-index to the sidebar (which only hides the issue, doesn't fix it), I wrap `<table>` elements inside a `<div>` and limit all chidren of `.docblock` elements' width to prevent having the scrollbar on the whole doc block.

![Screenshot from 2021-09-08 15-11-24](https://user-images.githubusercontent.com/3050060/132515740-71796515-e74f-429f-ba98-2596bdbf781c.png)

Thanks `@nbdd0121` for `overflow-x: auto;`. ;)

r? `@notriddle`
…ements_grid_bug, r=GuillaumeGomez

Workaround blink/chromium grid layout limitation of 1000 rows

I made this in case we don't come up with a better solution in time.

See rust-lang#88545 for more details.

A rendered version of the standard library is hosted here:
https://data.estada.ch/rustdoc-nightly_497ee321af_2021-09-09/core/arch/arm/index.html

r? `@GuillaumeGomez` `@jsha`
Fix typo `option` -> `options`.
@rustbot rustbot added the rollup A PR which is a rollup label Sep 10, 2021
@Manishearth
Copy link
Member Author

@bors r+ p=1

might as well get this ready for when the tree reopens

@bors
Copy link
Contributor

bors commented Sep 10, 2021

📌 Commit f77311b has been approved by Manishearth

@bors
Copy link
Contributor

bors commented Sep 10, 2021

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 10, 2021
@Manishearth
Copy link
Member Author

@bors p=3

should probabyl take some priority over submodule updates

@bors
Copy link
Contributor

bors commented Sep 11, 2021

⌛ Testing commit f77311b with merge 22719ef...

@bors
Copy link
Contributor

bors commented Sep 11, 2021

☀️ Test successful - checks-actions
Approved by: Manishearth
Pushing 22719ef to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 11, 2021
@bors bors merged commit 22719ef into rust-lang:master Sep 11, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 11, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (22719ef): comparison url.

Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.

  • Moderate improvement in instruction counts (up to -0.7% on incr-patched: println builds of html5ever)
  • Moderate regression in instruction counts (up to 1.0% on incr-unchanged builds of derive)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@rustbot rustbot added the perf-regression Performance regression. label Sep 11, 2021
@erikdesjardins
Copy link
Contributor

erikdesjardins commented Oct 2, 2021

Cachegrind diff of the derive-check incr-unchanged regression:

(using https://github.com/rust-lang/rustc-perf, cargo +nightly run --release --bin collector -- diff_local cachegrind --include derive --builds Check --runs IncrUnchanged +b69fe57261086e70aea9d5b58819a1794bf7c121 +22719efcc570b043f2e519d6025e5f36eab38fe2)

--------------------------------------------------------------------------------
Ir                  
--------------------------------------------------------------------------------
21,559,958 (100.0%)  PROGRAM TOTALS

--------------------------------------------------------------------------------
Ir                   file:function
--------------------------------------------------------------------------------
 7,195,928 (33.38%)  ???:rustc_middle::hir::map::Map::attrs
 4,298,100 (19.94%)  ???:rustc_session::session::Session::contains_name
 1,669,319 ( 7.74%)  ???:rustc_middle::hir::map::collector::NodeCollector::insert_owner
-1,434,995 (-6.66%)  ???:rustc_data_structures::stable_hasher::StableHasher::finish
   946,200 ( 4.39%)  ???:rustc_middle::ty::query::<impl rustc_middle::ty::context::TyCtxt>::def_kind
   945,437 ( 4.39%)  ???:rustc_middle::ty::<impl rustc_middle::ty::context::TyCtxt>::get_attrs
   932,400 ( 4.32%)  ???:rustc_middle::dep_graph::<impl rustc_query_system::dep_graph::DepKind for rustc_middle::dep_graph::dep_node::DepKind>::read_deps
   651,552 ( 3.02%)  ???:rustc_hir::hir::_DERIVE_rustc_data_structures_stable_hasher_HashStable_CTX_FOR_OwnerNode::<impl rustc_data_structures::stable_hasher::HashStable<__CTX> for rustc_hir::hir::OwnerNode>::hash_stable
  -582,932 (-2.70%)  ???:hashbrown::map::HashMap<K,V,S,A>::insert
   576,600 ( 2.67%)  ???:rustc_middle::ty::<impl rustc_middle::ty::context::TyCtxt>::opt_associated_item
   516,518 ( 2.40%)  ???:rustc_passes::dead::check_crate
   487,365 ( 2.26%)  ???:rustc_query_system::query::plumbing::get_query_impl
   473,618 ( 2.20%)  ???:rustc_middle::hir::map::collector::NodeCollector::insert_nested
   462,998 ( 2.15%)  ???:rustc_query_system::dep_graph::graph::DepGraph<K>::try_mark_previous_green
   395,432 ( 1.83%)  ???:rustc_query_system::query::plumbing::incremental_verify_ich
   378,602 ( 1.76%)  ???:rustc_query_system::dep_graph::serialized::GraphEncoder<K>::send
   322,351 ( 1.50%)  ???:<rustc_query_impl::on_disk_cache::OnDiskCache as rustc_middle::ty::context::OnDiskCache>::def_path_hash_to_def_id
   319,383 ( 1.48%)  ???:rustc_data_structures::stack::ensure_sufficient_stack
   318,220 ( 1.48%)  ???:rustc_query_system::query::plumbing::JobOwner<D,C>::complete
   302,127 ( 1.40%)  ???:rustc_serialize::serialize::Decoder::read_seq
  -295,013 (-1.37%)  ???:<rustc_middle::hir::Owner as rustc_data_structures::stable_hasher::HashStable<rustc_middle::ich::hcx::StableHashingContext>>::hash_stable
   280,800 ( 1.30%)  ???:rustc_middle::ty::<impl rustc_middle::ty::context::TyCtxt>::has_attr
   274,781 ( 1.27%)  ./string/../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:__memcpy_sse2_unaligned_erms
  -273,300 (-1.27%)  ???:rustc_hir::intravisit::walk_qpath
  -257,787 (-1.20%)  obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/build/tikv-jemalloc-sys-fde45a9a697cc078/out/build/src/arena.c:_rjem_je_arena_tcache_fill_small
  -249,000 (-1.15%)  ???:rustc_hir::intravisit::walk_expr
  -247,800 (-1.15%)  ???:<rustc_passes::dead::MarkSymbolVisitor as rustc_hir::intravisit::Visitor>::visit_expr
  -233,550 (-1.08%)  ???:rustc_passes::dead::MarkSymbolVisitor::handle_res
   231,961 ( 1.08%)  ???:scoped_tls::ScopedKey<T>::with
   226,893 ( 1.05%)  ???:rustc_middle::ty::<impl rustc_middle::ty::context::TyCtxt>::trait_id_of_impl
   217,508 ( 1.01%)  ???:<smallvec::SmallVec<A> as core::iter::traits::collect::Extend<<A as smallvec::Array>::Item>>::extend
...

attrs/contains_name/etc. points to #85200.

It looks like some of the attribute checks added in that PR are unused, I'll open a PR to remove them and see if it helps.

Edit: opened #89454

erikdesjardins added a commit to erikdesjardins/rust that referenced this pull request Oct 2, 2021
The checks removed here caused a small perf regression:
rust-lang#88824 (comment)

Since the attribute is currently only applied to traits, I don't think
it's worth keeping the additional checks for now.
If/when we decide to apply the attribute somewhere else, we can
(partially) revert this and evaluate if the perf impact is acceptable.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 7, 2021
…atsakis

perf: only check for `rustc_trivial_field_reads` attribute on traits, not items, impls, etc.

The checks that are removed in this PR (originally added in rust-lang#85200) caused a small perf regression: rust-lang#88824 (comment)

Since the attribute is currently only applied to traits, I don't think it's worth keeping the additional checks for now.
If/when we decide to apply the attribute somewhere else, we can (partially) revert this and reevaluate the perf impact.

r? `@nikomatsakis` cc `@FabianWolff`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. rollup A PR which is a rollup S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet