Rollup of 5 pull requests #128169

matthiaskrgr · 2024-07-25T02:43:21Z

Successful merges:

Reorder trait bound modifiers *after* for<...> binder in trait bounds #127054 (Reorder trait bound modifiers after for<...> binder in trait bounds)
Replace ASCII control chars with Unicode Control Pictures #127528 (Replace ASCII control chars with Unicode Control Pictures)
Migrate pointer-auth-link-with-c, c-dynamic-rlib and c-dynamic-dylib run-make tests to rmake #127872 (Migrate pointer-auth-link-with-c, c-dynamic-rlib and c-dynamic-dylib run-make tests to rmake)
Do not use question as label #128111 (Do not use question as label)
Don't ICE when auto trait has assoc ty in old solver #128160 (Don't ICE when auto trait has assoc ty in old solver)

r? @ghost
@rustbot modify labels: rollup

``` error: bare CR not allowed in doc-comment --> $DIR/lex-bare-cr-string-literal-doc-comment.rs:3:32 | LL | /// doc comment with bare CR: '␍' | ^ ```

No longer track "zero-width" chars in `SourceMap`, read directly from the line when calculating the `display_col` of a `BytePos`. Move `char_width` to `rustc_span` and use it from the emitter. This change allows the following to properly align in terminals (depending on the font, the replaced control codepoints are rendered as 1 or 2 width, on my terminal they are rendered as 1, on VSCode text they are rendered as 2): ``` error: this file contains an unclosed delimiter --> $DIR/issue-68629.rs:5:17 | LL | ␜␟ts␀![{i | -- unclosed delimiter | | | unclosed delimiter LL | ␀␀ fn rݻoa>rݻm | ^ ```

We already point these out quite aggressively, telling people not to use them, but would normally be rendered as nothing. Having them visible will make it easier for people to actually deal with them. ``` error: unicode codepoint changing visible direction of text present in literal --> $DIR/unicode-control-codepoints.rs:26:22 | LL | println!("{:?}", '�'); | ^-^ | || | |'\u{202e}' | this literal contains an invisible unicode text flow control codepoint | = note: these kind of unicode codepoints change the way text flows on applications that support them, but can cause confusion because they change the order of characters on the screen = help: if their presence wasn't intentional, you can remove them help: if you want to keep them but make them visible in your source code, you can escape them | LL | println!("{:?}", '\u{202e}'); | ~~~~~~~~ ``` vs the previous ``` error: unicode codepoint changing visible direction of text present in literal --> $DIR/unicode-control-codepoints.rs:26:22 | LL | println!("{:?}", ''); | ^- | || | |'\u{202e}' | this literal contains an invisible unicode text flow control codepoint | = note: these kind of unicode codepoints change the way text flows on applications that support them, but can cause confusion because they change the order of characters on the screen = help: if their presence wasn't intentional, you can remove them help: if you want to keep them but make them visible in your source code, you can escape them | LL | println!("{:?}", '\u{202e}'); | ~~~~~~~~ ```

We don't want to have questions in the diagnostic output. Instead, we use wording that communicates uncertainty, like "might": ``` error[E0432]: unresolved import `spam` --> $DIR/import-from-missing-star-3.rs:2:9 | LL | use spam::*; | ^^^^ you might be missing crate `spam` | = help: consider adding `extern crate spam` to use the `spam` crate ```

…fmease Reorder trait bound modifiers *after* `for<...>` binder in trait bounds This PR suggests changing the grammar of trait bounds from: ``` [CONSTNESS] [ASYNCNESS] [?] [BINDER] [TRAIT_PATH] const async ? for<'a> Sized ``` to ``` ([BINDER] [CONSTNESS] [ASYNCNESS] | [?]) [TRAIT_PATH] ``` i.e., either ``` ? Sized ``` or ``` for<'a> const async Sized ``` (but not both) ### Why? I think it's strange that the binder applies "more tightly" than the `?` trait polarity. This becomes even weirder when considering that we (or at least, I) want to have `async` trait bounds expressed like: ``` where T: for<'a> async Fn(&'a ()) -> i32, ``` and not: ``` where T: async for<'a> Fn(&'a ()) -> i32, ``` ### Fallout No crates on crater use this syntax, presumably because it's literally useless. This will require modifying the reference grammar, though. ### Alternatives If this is not desirable, then we can alternatively keep parsing `for<'a>` after the `?` but deprecate it with either an FCW (or an immediate hard error), and begin parsing `for<'a>` *before* the `?`.

…i-obk Replace ASCII control chars with Unicode Control Pictures Replace ASCII control chars like `CR` with Unicode Control Pictures like `␍`: ``` error: bare CR not allowed in doc-comment --> $DIR/lex-bare-cr-string-literal-doc-comment.rs:3:32 | LL | /// doc comment with bare CR: '␍' | ^ ``` Centralize the checking of unicode char width for the purposes of CLI display in one place. Account for the new replacements. Remove unneeded tracking of "zero-width" unicode chars, as we calculate these in the `SourceMap` as needed now.

…jieyouxu Migrate `pointer-auth-link-with-c`, `c-dynamic-rlib` and `c-dynamic-dylib` `run-make` tests to rmake Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html). Please try: try-job: x86_64-msvc try-job: i686-mingw try-job: aarch64-apple

Do not use question as label We don't want to have questions in the diagnostic output. Instead, we use wording that communicates uncertainty, like "might": ``` error[E0432]: unresolved import `spam` --> $DIR/import-from-missing-star-3.rs:2:9 | LL | use spam::*; | ^^^^ you might be missing crate `spam` | = help: consider adding `extern crate spam` to use the `spam` crate ```

Don't ICE when auto trait has assoc ty in old solver Kinda a pointless change to make, but it's observable w/o the feature gate, so let's just fix it. I reintroduced this ICE when I removed the "auto impl" kind from `ImplSource` in rust-lang#112687. Fixes rust-lang#117829 Fixes rust-lang#127746

matthiaskrgr · 2024-07-25T02:43:58Z

@bors r+ rollup=never p=5

bors · 2024-07-25T02:44:01Z

📌 Commit 1fda084 has been approved by matthiaskrgr

It is now in the queue for this repository.

bors · 2024-07-25T04:17:51Z

⌛ Testing commit 1fda084 with merge 004e155...

bors · 2024-07-25T06:39:53Z

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

rust-timer · 2024-07-25T06:41:21Z

📌 Perf builds for each rolled up PR:

PR#	Message	Perf Build Sha
#127054	Reorder trait bound modifiers after `for<...>` binder in …	`0d5c969dd048d26e1488db4ecee869b4f484d396` (link)
#127528	Replace ASCII control chars with Unicode Control Pictures	`e3343bd96d6125c21f3e32f4be744b7c026ec6b2` (link)
#127872	Migrate `pointer-auth-link-with-c`, `c-dynamic-rlib` and `c…	`f64425d7ee67c78ef46cdf664543bf66cefa626f` (link)
#128111	Do not use question as label	`cf2bcb74fabea8c0bf6a31206d3d304fb663debc` (link)
#128160	Don't ICE when auto trait has assoc ty in old solver	`e2df67abee9c3dfc313599cde24e7d9136a0db3c` (link)

previous master: e7d66eac5e

In the case of a perf regression, run the following command for each PR you suspect might be the cause: @rust-timer build $SHA

rust-timer · 2024-07-25T07:57:35Z

Finished benchmarking commit (004e155): 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)	0.9%	[0.2%, 3.0%]	26
Regressions ❌ (secondary)	0.5%	[0.3%, 2.2%]	13
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.9%	[0.2%, 3.0%]	26

Max RSS (memory usage)

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

Cycles

Results (primary 1.6%, secondary 8.2%)

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)	1.6%	[1.4%, 1.8%]	5
Regressions ❌ (secondary)	8.2%	[7.4%, 9.5%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.6%	[1.4%, 1.8%]	5

Binary size

Results (secondary -0.0%)

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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	1
All ❌✅ (primary)	-	-	0

Bootstrap: 771.74s -> 771.005s (-0.10%)
Artifact size: 328.91 MiB -> 328.90 MiB (-0.00%)

Kobzol · 2024-07-25T11:19:56Z

#127528 was the culprit, it is being reverted in #128179.

lqd · 2024-07-25T11:49:53Z

isn't that PR only for diagnostics? ie indicating that the benchmarks produce warnings, but are not real slowdowns of the compiler?

Kobzol · 2024-07-25T11:51:50Z

Is is possible, although the regression was bad enough that I think a revert and a follow-up investigation of the perf result is mostly warranted.

On a related note, it would be good to know the size of the diagnostics outputted by our current benchmarks, it's quite probable that it is excessive. I will try to take a look at it later.

lqd · 2024-07-25T12:18:19Z

With all the new lints, and changes to existing ones, it wouldn't be surprising that there are enough that diagnostics changes "impact benchmarks but not performance". That ties to the topic of doing something about this measurement behavior, which was discussed on zulip a while back.

estebank · 2024-07-25T16:52:17Z

I'm catching up on this. I am surprised that the benchmarks are affected because in my mental model they were not affected by slowdowns on the diagnostics machinery itself. I can see this been an actual regression on the time it takes to render diagnostics, because now we are unconditionally calculating the character width for underlines, whereas before that was kept in an unconditionally pre-calculated table. The new behavior is more correct, but we'd have to see how/if we can mitigate the regression. I noticed that syn is one of the most significantly impacted, which in my head points at TokenString rendering, maybe. I would love some help actually understanding what actually regressed, because looking at the perf summaries I'm seeing codegen_copy_artifacts_from_incr_cache, early_lint_checks and LLVM_passes highlighted as affected places, where the last one I don't see how it could possibly be affected, and the first one should be faster because we're storing less info...

Edit:

Is it the case that the syn perf tests are emitting lint warnings to stderr when they get compiled? If that is the case, then I can see where the perf regression might be coming from. They would be representative of the new cost of diagnostic rendering, but it is not representative of the cost of compiling syn as part of a dependency tree (because of the implicit cap-lints we have for dependencies). If so, then I don't think it is reasonable to completely block any regressions on the rendering speed when this change is both a QoL improvement and a correctness fix. After the revert lands I will try to split the change into more granular changes to see where the regression is introduced exactly, but it feels "wrong" to have the same rigor for compile changes in the happy path (code that is "perfect") and for the "unhappy" path (code with diagnostics), particularly because for the latter the unofficial policy is "as long as it doesn't affect the happy path, we're ok with doing more work in the unhappy path".

If I can confirm that there's not some "hidden"/unconditional place where col_display is getting calculated that shouldn't when it comes to proc-macros (it can't be for everything because otherwise we would have seen regressions on every perf test), I would strongly make the case that we should land the whole change after an attempt to bring the impact down as much as possible, but we can't not fix incorrect output purely because calculating the output gets single digit percent slower.

lqd · 2024-07-26T10:09:41Z

I would love some help actually understanding what actually regressed

In the benchmark results, when you click on one there's a graph to help you see if the change could be noise, and a command at the bottom to run a cachegrind diff locally, which helps see where the slowdowns come from.

For example for the syn incr-unchanged one, cachegrind shows the following for me:

--------------------------------------------------------------------------------
Ir
--------------------------------------------------------------------------------
52,077,877  PROGRAM TOTALS

--------------------------------------------------------------------------------
Ir           file:function
--------------------------------------------------------------------------------
 14,769,207  library/core/src/slice/memchr.rs:core::slice::memchr::memchr_aligned
 14,357,444  ???:<rustc_parse::parser::Parser>::collect_tokens_for_expr::<<rustc_parse::parser::Parser>::parse_expr_dot_or_call::{closure#0}>::{closure#0}
-10,438,596  ???:<rustc_parse::parser::Parser>::parse_expr_dot_or_call::{closure#0}
 10,289,602  ???:rustc_errors::emitter::normalize_whitespace
  9,335,170  <all-jemalloc-files>:<all-jemalloc-functions>
  4,937,003  ???:<rustc_errors::emitter::HumanEmitter>::emit_messages_default_inner::{closure#0}
 -4,596,152  ???:<rustc_parse::parser::Parser>::parse_path_inner
  3,466,792  ???:alloc::raw_vec::finish_grow::<alloc::alloc::Global>
  2,927,330  ???:<rustc_span::source_map::SourceMap>::lookup_char_pos
  2,498,800  ???:???
...

As you expected: more string searching, slower whitespace normalization, more memory allocations, etc.

And since I similarly was confused by the results, yes, syn-1.0.89 currently generates a lot of warnings.

warning: `syn` (lib) generated 514 warnings (90 duplicates)

That IMHO makes this not really a regression, nor needing a revert. #128200 gains some perf back, it's still doing more work than prior to #127528 and that is perfectly fine for diagnostics.

Kobzol · 2024-07-26T10:11:45Z

Thanks a lot for the analysis! I agree that especially with #128200, the revert is not needed.

pnkfelix · 2024-07-31T14:10:43Z

the bulk of the regressions are to syn (i.e. 8 out of the 9 that are > 1%).
this was due to a change in how diagnostics handle certain "whitespace" characters (PR Replace ASCII control chars with Unicode Control Pictures #127528); there is a revert proposed in PR Revert Replace ASCII control chars with Unicode Control Pictures #128179, but there is also a PR to address the issue itself as a followup in PR Change output normalization logic to be linear against size of output #128200
not marking as triaged until either PR Revert Replace ASCII control chars with Unicode Control Pictures #128179 or PR Change output normalization logic to be linear against size of output #128200 is landed.

compiler-errors and others added 20 commits July 10, 2024 17:15

Reorder modifiers and polarity to be *after* binder in trait bounds

e844581

Improve error message

32c8bfd

Enforce that ? and for<...> are not combined

898ed2f

And additionally enforce ? and async/const aren't mixed

de88bc5

Remove rustdoc tests which no longer parse

79d1ac6

Replace ASCII control chars with Unicode Control Pictures

89f273f

``` error: bare CR not allowed in doc-comment --> $DIR/lex-bare-cr-string-literal-doc-comment.rs:3:32 | LL | /// doc comment with bare CR: '␍' | ^ ```

rewrite pointer-auth-link-with-c to rmake

ef07fe1

rewrite c-dynamic-rlib to rmake

63286d8

rewrite c-dynamic-dylib to rmake

710e7b0

Fix ddltool-failed test

9bd7680

Don't ICE when auto trait has assoc ty in old solver

0919d07

Apply suggestions from code review

7da751a

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 Jul 25, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 25, 2024

bors merged commit 004e155 into rust-lang:master Jul 25, 2024
7 checks passed

rustbot added this to the 1.82.0 milestone Jul 25, 2024

bors mentioned this pull request Jul 25, 2024

Migrate foreign-double-unwind, issue-36710 and foreign-exceptions run-make tests to rmake #127926

Merged

rustbot added the perf-regression Performance regression. label Jul 25, 2024

compiler-errors mentioned this pull request Jul 25, 2024

Revert Replace ASCII control chars with Unicode Control Pictures #128179

Closed

GrigorenkoPV mentioned this pull request Aug 6, 2024

rustc_errors: use perfect hashing for character replacements #128463

Closed

matthiaskrgr deleted the rollup-ylsoq30 branch September 1, 2024 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rollup of 5 pull requests #128169

Rollup of 5 pull requests #128169

matthiaskrgr commented Jul 25, 2024

matthiaskrgr commented Jul 25, 2024

bors commented Jul 25, 2024

bors commented Jul 25, 2024

bors commented Jul 25, 2024

rust-timer commented Jul 25, 2024

rust-timer commented Jul 25, 2024

Kobzol commented Jul 25, 2024

lqd commented Jul 25, 2024

Kobzol commented Jul 25, 2024

lqd commented Jul 25, 2024 •

edited

Loading

estebank commented Jul 25, 2024 •

edited

Loading

lqd commented Jul 26, 2024

Kobzol commented Jul 26, 2024

pnkfelix commented Jul 31, 2024

Rollup of 5 pull requests #128169

Rollup of 5 pull requests #128169

Conversation

matthiaskrgr commented Jul 25, 2024

matthiaskrgr commented Jul 25, 2024

bors commented Jul 25, 2024

bors commented Jul 25, 2024

bors commented Jul 25, 2024

rust-timer commented Jul 25, 2024

rust-timer commented Jul 25, 2024

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Kobzol commented Jul 25, 2024

lqd commented Jul 25, 2024

Kobzol commented Jul 25, 2024

lqd commented Jul 25, 2024 • edited Loading

estebank commented Jul 25, 2024 • edited Loading

lqd commented Jul 26, 2024

Kobzol commented Jul 26, 2024

pnkfelix commented Jul 31, 2024

lqd commented Jul 25, 2024 •

edited

Loading

estebank commented Jul 25, 2024 •

edited

Loading