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 9 pull requests #108488

Merged
merged 23 commits into from
Feb 26, 2023
Merged

Rollup of 9 pull requests #108488

merged 23 commits into from
Feb 26, 2023

Conversation

matthiaskrgr
Copy link
Member

Successful merges:

Failed merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

scottmcm and others added 23 commits February 22, 2023 23:26
The point of these is to be seen lexically in the docs, so they should always be passed as the correct literal, not as an expression.

(Otherwise we could just compute `Min`/`Max` from `BITS`, for example.)
This resolves an inconsistency in naming style for functions
on the parser, between functions parsing specific kinds of items
and those for expressions, favoring the parse_item_[sth] style
used by functions for items. There are multiple advantages
of that style:

* functions of both categories are collected in the same place
  in the rustdoc output.
* it helps with autocompletion, as you can narrow down your
  search for a function to those about expressions.
* it mirrors rust's path syntax where less specific things
  come first, then it gets more specific, i.e.
  std::collections::hash_map::Entry

The disadvantage is that it doesn't "read like a sentence"
any more, but I think the advantages weigh more greatly.

This change was mostly application of this command:

sed -i -E 's/(fn |\.)parse_([[:alnum:]_]+)_expr/\1parse_expr_\2/' compiler/rustc_parse/src/parser/*.rs

Plus very minor fixes outside of rustc_parse, and an invocation
of x fmt.
This adds both a test specific to rust-lang#108453 as well as an exhaustive test
that goes through all possible combinations of head index, length and target capacity
for a deque with capacity 16.
It was probably a leftover from the old `?` desugaring but anyways, it's
unused now except for clippy, which can just use a diagnostics item.
…or-auto, r=lcnr

Treat `str` as containing `[u8]` for auto trait purposes

Wanted to gauge ``@rust-lang/lang`` and ``@rust-lang/types`` teams' thoughts on treating `str` as "containing" a `[u8]` slice for auto-trait purposes.

``@dtolnay`` brought this up in rust-lang#13231 (comment) as a blocker for future `str` type librarification, and I think it's both a valid concern and very easy to fix. I'm interested in actually doing that `str` type librarification (rust-lang#107939), but this probably should be considered in the mean time regardless of that PR.

r? types for the impl, though this definitely needs an FCP.
Require `literal`s for some `(u)int_impl!` parameters

The point of these is to be seen *lexically* in the docs, so they should always be passed as the correct literal, not as an expression.

(Otherwise we could just compute `Min`/`Max` from `BITS`, for example.)

r? Nilstrieb
…, r=cjgillot

hir-analysis: make a helpful note
…ed, r=cjgillot

Add `ErrorGuaranteed` to `hir::{Expr,Ty}Kind::Err` variants

First step in making the `Err` variants of `ExprKind` and `TyKind` require an `ErrorGuaranteed` during parsing. Making the corresponding AST versions require `ErrorGuaranteed` is a bit harder, whereas it was pretty easy to do this for HIR, so let's do that first.

The only weird thing about this PR is that `ErrorGuaranteed` is moved to `rustc_span`. This is *certainly* not the right place to put it, but `rustc_hir` cannot depend on `rustc_error` because the latter already depends on the former. Should I just pull out some of the error machinery from `rustc_error` into an even more minimal crate that `rustc_hir` can depend on? Advice would be appreciated.
…strieb

Replace parse_[sth]_expr with parse_expr_[sth] function names

This resolves an inconsistency in naming style for functions on the parser, where:

* functions parsing specific kinds of items are named `parse_item_[sth]` and
* functions parsing specific kinds of *expressions* are named `parse_[sth]_expr`

favoring the style used by functions for items. There are multiple advantages of that style:

* functions of both categories are collected in the same place in the [rustdoc output](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_parse/parser/struct.Parser.html).
* it helps with autocompletion, as you can narrow down your search for a function to those about expressions.
* it mirrors rust's path syntax where less specific things come first, then it gets more specific, i.e. `std::collections::hash_map::Entry`.

The disadvantage is that it doesn't "read like a sentence" any more. But I think the advantages weigh more greatly.

This change was mostly application of this command:

```
sed -i -E 's/(fn |\.)parse_([[:alnum:]_]+)_expr/\1parse_expr_\2/' compiler/rustc_parse/src/parser/*.rs
```

Plus very minor fixes outside of `rustc_parse`, and an invocation of `x fmt`.
…ompiler-errors

rustc_infer: Consolidate obligation elaboration de-duplication

# Explanation

The obligations `Elaborator` is doing de-duplication of obligations in 3 different locations. 1 off which has a comment.
This PR consolidates the functionality and comment to a single function.
Fix `VecDeque::shrink_to` and add tests.

Fixes rust-lang#108453.

Also adds both a specific test with the code from rust-lang#108453 and an exhaustive test that checks all possible head positions, lengths and target capacities for deques with capacity 16.

cc `@trinity-1686a` `@scottmcm`
…aumeGomez

statically guarantee that current error codes are documented

Closes rust-lang#61137 (that's right!)

Pretty simple refactor (often just a change from `Result<Option<&str>>` to `Result<&str>`)

r? `@GuillaumeGomez` (could you specially look at 5304415? I believe you wrote that in the first place, just want to make sure you're happy with the change)
…as From˂˂LangItemFromFn˃˃˃꞉꞉from, r=cjgillot

Remove `from` lang item

It was probably a leftover from the old `?` desugaring but anyways, it's unused now except for clippy, which can just use a diagnostics item.
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) rollup A PR which is a rollup labels Feb 26, 2023
@matthiaskrgr
Copy link
Member Author

@bors r+ rollup=never p=9

@bors
Copy link
Contributor

bors commented Feb 26, 2023

📌 Commit 9c27fc7 has been approved by matthiaskrgr

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 26, 2023
@bors
Copy link
Contributor

bors commented Feb 26, 2023

⌛ Testing commit 9c27fc7 with merge c4e0cd9...

@bors
Copy link
Contributor

bors commented Feb 26, 2023

☀️ Test successful - checks-actions
Approved by: matthiaskrgr
Pushing c4e0cd9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 26, 2023
@bors bors merged commit c4e0cd9 into rust-lang:master Feb 26, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 26, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c4e0cd9): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

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
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.4% [0.8%, 2.0%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.4% [0.8%, 2.0%] 2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.2% [4.2%, 4.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.4% [2.4%, 2.4%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

@lqd
Copy link
Member

lqd commented Feb 27, 2023

These are doc benchmarks regressions, and nothing obviously rustdoc jumps out. Maybe it's noise but we don't seem to show rustdoc benchmark graphs on perf.rlo.

Just for peace of mind, let's see if #108299 is related since it seems to be about doc comments.

@rust-timer build 43795ad03b0e98aea2c37fb5869a824d501124f6

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (43795ad03b0e98aea2c37fb5869a824d501124f6): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.6% [3.6%, 3.6%] 1
Improvements ✅
(primary)
-3.8% [-3.8%, -3.8%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -3.8% [-3.8%, -3.8%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.7% [3.7%, 3.7%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.9% [-2.9%, -2.9%] 1
All ❌✅ (primary) - - 0

@lqd
Copy link
Member

lqd commented Feb 27, 2023

@rust-timer build 425b8cfc33a2518b3d8107cc31be92ae5ec8a999

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (425b8cfc33a2518b3d8107cc31be92ae5ec8a999): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

@pnkfelix
Copy link
Member

pnkfelix commented Mar 1, 2023

The regression here seems limited in scope. It is unfortunate, but I do not think the 2% regression to building docs for hyper on its own warrants trying to dissect what happened here.

I do think we should probably add some way to see graphs of the doc-build time data. I'll go file an issue for that rustc-perf.

(Also, it seems bad that the runs on the individual commits are removing the perf-regression label here... -- or ... is that happening? Why don't I see the label modifications that I would expect in response to the rust-timer comment above...?)

@rustbot label: +perf-regression +perf-regression-triaged

@rustbot rustbot added perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. labels Mar 1, 2023
@nnethercote
Copy link
Contributor

(Also, it seems bad that the runs on the individual commits are removing the perf-regression label here... -- or ... is that happening? Why don't I see the label modifications that I would expect in response to the rust-timer comment above...?)

The perf-regression removal happens when a perf run is made and no regressions are detected. That works fine in the normal case, but here when doing perf runs on individual commits within a rollup it can lead to the overall PR being marked as a non-regression simply because a single perf run was non-regressing. Fixing it would probably require teaching rustc-perf to handle individual runs within rollups separately.

@pnkfelix
Copy link
Member

pnkfelix commented Mar 1, 2023

Fixing it would probably require teaching rustc-perf to handle individual runs within rollups separately.

Yes, this is along the lines of what I was thinking

@Kobzol
Copy link
Contributor

Kobzol commented Mar 5, 2023

The perf-regression removal happens when a perf run is made and no regressions are detected. That works fine in the normal case, but here when doing perf runs on individual commits within a rollup it can lead to the overall PR being marked as a non-regression simply because a single perf run was non-regressing. Fixing it would probably require teaching rustc-perf to handle individual runs within rollups separately.

This now works: #108747 (comment)
Running perf bot on merged rollup PRs should no longer set or remove any regression tags.

@matthiaskrgr matthiaskrgr deleted the rollup-i61epcw branch March 16, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.