Further improve space_between

[nnethercote]

space_between is used by print_tts to decide when spaces should be put between tokens. This PR improves it in two ways:

• avoid unnecessary spaces before semicolons, and
• don't omit some necessary spaces before/after some punctuation symbols.

r? @petrochenkov

[nnethercote]

#117433 was an attempt to make space_between much cleverer, which didn't work out. This PR is a greatly cut down version of that, which gets some nice improvements with very little extra complexity.

It will need a crater run before being merged, but based on the crater run from #117433 I think it should be ok, because I haven't include the cases that caused crater failures in #117433, which were mostly about spacing around : and :: tokens.

[petrochenkov]

@bors try

[bors]

⌛ Trying commit 1fbabee with merge ad2e388...

[bors]

☀️ Try build successful - checks-actions
Build commit: ad2e388 (ad2e3884fa8975e4827c1b0b0af203fc59856ef2)

[petrochenkov]

@craterbot check

[craterbot]

👌 Experiment pr-120227 created and queued.
🤖 Automatically detected try build ad2e388
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-120227 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-120227 is completed!
📊 6 regressed and 2 fixed (409984 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[petrochenkov]

Only 2 regressions out of 6 are spurious.
@rustbot author

[nnethercote]

odbc-futures-0.2.2

This line is the problem, it does substring matching with "extern crate crate ;", and there is no longer a space before the semicolon.

The crate has only 438 downloads ever and hasn't been updated in 5 years.

KittyCAD.kcl-lsp.844488870375d9e36254825ab04a2cfe5d5fd107

Due to kcl-lib/derive-docs, as described here. In short, this:

Result < Box < ExtrudeGroup >, KclError > {}

becomes this:

Result < Box < ExtrudeGroup > , KclError > {}

which breaks some string matching in a proc macro.

kcl-lib and derive-docs have had both had updates in the past two months.

carghai.contain.be6f4b896c7594bac76ae43a2027ed7dd602a433

Due to const-random use, seen before here, so I'm surprised it's showing up here.

[INFO] [stdout] error: proc macro panicked
[INFO] [stdout]   --> src/encryption/base.rs:11:32
[INFO] [stdout]    |
[INFO] [stdout] 11 | const RANDOM_BYTES: [u8; 16] = const_random!([u8 ; 16]);

gen-nested-iter-yield-0.1.3

Substring matching done here in the nested_iter_yield proc macro. I'm not sure the exact problem, probably . or , changes.

Crate has 3,376 total downloads ever, and was last updated almost 2 years ago.

Malikazz.ProjectEuler.81ee5516c17a5425ef7b0b2dbf8ef010031deefc, tperdue321.rust_structs.c5244df34b978d5d014720c1fa42514cc89a7c37, Mallig.advent-of-code-2020.27889bb9ca6c8080172bef594543c49e52fc524c (fixed)

Error response from daemon: failed to create task for container: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: unable to apply cgroup configuration: unable to start unit "docker-937d68c71184d5335f9a4e5480e9d18bbcc3b5922aec6d9c73612f74bbdad7d2.scope

Seems unrelated to this PR, looks like temporary infra problems.

lcdr.lu_packets.144e56d2c46043da7924ffa34705f763982676fc (fixed)

error[E0275]: overflow evaluating the requirement `BitWriter<endio_bit::endian::BigEndian, _>: std::io::Write`
  |
  = help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`lu_packets`)
  = note: required for `BitWriter<endio_bit::endian::BigEndian, _>` to implement `EWrite<LittleEndian>`
  = note: required for `&'a [_]` to implement `for<'a> Serialize<LittleEndian, BitWriter<endio_bit::endian::BigEndian, _>>`
  = note: 125 redundant requirements hidden
  = note: required for `&Option<[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[...]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]>` to implement `Serialize<LittleEndian, BitWriter<endio_bit::endian::BigEndian, _>>`
  = note: the full type name has been written to '/opt/rustwide/target/debug/deps/lu_packets-8f97e4ad4ec14e69.long-type-5894850763885052302.txt'

Old failure was due to hitting the recursion limit. Unclear why it worked now, but seems unrelated to this PR.

[nnethercote]

carghai.contain.be6f4b896c7594bac76ae43a2027ed7dd602a433

Due to const-random use, seen before here, so I'm surprised it's showing up here.

This would make sense if crater is comparing this PR against release Rust, rather than against nightly Rust.

I filed tkaitchuck/constrandom#33 to fix const-random, making the proc macro iterate over the token trees rather than using to_string and then matching substrings. This seemed a reasonable way to do it, but I've never done it before, so I'd be happy to hear if there is a better way. (The use of syn seems like overkill for that proc macro, because it's very simple.)

[nnethercote]

@petrochenkov: with tkaitchuck/constrandom#33 filed, I think the only other breakage that matters is the kcl-lib/derive-docs case, where both crates are written by the same people.

I suggest we merge this PR and then I'll file an issue on derive-docs. And then perhaps add something to the release docs, as per #119875.

@rustbot reviewer

[petrochenkov]

Okay, seems reasonable.
@bors r+

[bors]

📌 Commit 1fbabee has been approved by petrochenkov

It is now in the queue for this repository.

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[nnethercote]

@bors retry

[bors]

⌛ Testing commit 1fbabee with merge 80deabd...

[bors]

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 80deabd to master...

[rust-timer]

Finished benchmarking commit (80deabd): 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)	3.8%	[3.8%, 3.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.8%	[3.8%, 3.8%]	1

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)	2.3%	[0.6%, 3.8%]	46
Regressions ❌ (secondary)	3.0%	[0.7%, 5.2%]	77
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.3%	[0.6%, 3.8%]	46

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

Binary size

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

Bootstrap: 660.046s -> 661.683s (0.25%)
Artifact size: 308.08 MiB -> 308.09 MiB (0.01%)

[nnethercote]

I filed an issue on derive-docs: KittyCAD/modeling-app#1339

[therealprof]

@nnethercote https://perf.rust-lang.org/compare.html?start=cb4d9a1902b3ea17e93872dafb76d24aa6295c47&end=80deabd0987201e1b8d060400f50e03309a0105e&stat=max-rss doesn't look great to me... Is the noticable increase in RSS expected?

[Kobzol]

(Discussion about that ongoing here).

[Mark-Simulacrum]

Tagging relnotes for future compat notice. See #120227 (comment) for crater.

[trevyn]

This is hitting my turbosql crate, I think because of matching on ; as above: https://github.com/trevyn/turbosql/actions/runs/7735113959/job/21090247508

[nnethercote]

This is hitting my turbosql crate, I think because of matching on ; as above: https://github.com/trevyn/turbosql/actions/runs/7735113959/job/21090247508

Looking at the code, I would guess these regexes are to blame.

For the first regex, changing this:

^Option < \[u8 ; \d+\] >$

to this:

^Option < \[u8\s*; \d+\] >$

should fix the problem caused by this particular PR. Changing it to this:

^Option\s*<\s*\[\s*u8\s*;\s*\d+\s*\]\s*>$

would be more robust against possible future pretty-printing changes. But the awfulness of that regex shows the limits of substring matching for this purpose. The fully robust way to do this kind of thing is at the token level, like I did in tkaitchuck/constrandom#33.