`rustc_span` cleanups #117507

nnethercote · 2023-11-02T05:44:39Z

Just some things I found while looking over this crate.

r? @oli-obk

rustbot · 2023-11-02T05:44:49Z

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

compiler/rustc_span/src/lib.rs

compiler/rustc_span/src/fatal_error.rs

The comment just below the first one describes how the `impl !Send for FatalError` makes it impossible to `panic!(FatalError)`. And the second one should be `panic_any` instead of `panic!`.

And remove dead functions revealed by this.

nnethercote · 2023-11-02T06:24:33Z

I have updated to address the review comments so far.

r? @Nilstrieb

Most notably, this commit changes the `pub use crate::*;` in that file to `use crate::*;`. This requires a lot of `use` items in other crates to be adjusted, because everything defined within `rustc_span::*` was also available via `rustc_span::source_map::*`, which is bizarre. The commit also removes `SourceMap::span_to_relative_line_string`, which is unused.

These are all called very rarely, so there is no need for them to be inline.

With `create_default_session_globals_then`, which is preferable when it is appropriate.

Noratrieb · 2023-11-02T20:47:40Z

compiler/rustc_hir/src/tests.rs

@@ -14,7 +14,7 @@ fn def_path_hash_depends_on_crate_id() {
    // the crate by changing the crate disambiguator (e.g. via bumping the
    // crate's version number).

-    create_session_if_not_set_then(Edition::Edition2024, |_| {
+    create_session_globals_then(Edition::Edition2024, || {


wouldn't this panic if a second test was added?

Only if a create_session_globals_then gets nested within another one.

You can see test files like compiler/rustc_expand/src/parse/tests.rs have multiple tests, each one containing a create_default_session_globals_then call.

I don't much like the if_not_set_then functions for this reason. They only exist because there are some places where initialization is messy and there are code paths that are sometimes executed within a create_session_globals_then and sometimes not, depending on whether it's rustc vs. rustdoc. vs. whatever else. They could probably be removed with some effort.

Noratrieb

one question, r=me on the other 8 commits

Noratrieb · 2023-11-03T07:32:55Z

@bors r+

bors · 2023-11-03T07:32:57Z

📌 Commit 6358411 has been approved by Nilstrieb

It is now in the queue for this repository.

bors · 2023-11-03T14:57:44Z

⌛ Testing commit 6358411 with merge 9c20ddd...

bors · 2023-11-03T16:51:12Z

☀️ Test successful - checks-actions
Approved by: Nilstrieb
Pushing 9c20ddd to master...

rust-timer · 2023-11-03T19:32:37Z

Finished benchmarking commit (9c20ddd): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

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.4%	[0.4%, 0.4%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.4%, 0.4%]	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)	3.2%	[3.2%, 3.2%]	1
Improvements ✅ (primary)	-3.1%	[-3.1%, -3.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.1%	[-3.1%, -3.1%]	1

Cycles

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

Binary size

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

Bootstrap: 635.543s -> 636.863s (0.21%)
Artifact size: 304.59 MiB -> 304.49 MiB (-0.04%)

Update to the latest Rust toolchain (2023-11-06). The relevant changes are: - rust-lang/rust#117507: this required changing the import of `Span` from `rustc_span::source_map::Span` to `rustc_span::Span`. - rust-lang/rust#114208: this changed the data field for the `OffsetOf` variant of `NullOp` from `List<FieldIdx>` to `List<(VariantIdx, FieldIdx)>`, which required updating the relevant code in `rvalue.rs`. - rust-lang/rust#115626: the unchecked shift operators have been separated from the `unchecked_math` feature, so this required changing the feature annotation in `tests/ui/should-panic-attribute/unexpected-failures/test.rs` - Some rustc change (not sure which one) result in a line in `tests/coverage/unreachable/variant/main.rs` getting optimized out. To maintain what this test is testing, I changed the `match` to make it a bit less-prone to optimization. - A change in `cargo` (rust-lang/cargo#12779) resulted in an update to Kani's workspace `Cargo.toml` when `cargo add` is executed inside `tests/script-based-pre/build-cache-bin`. This is apparently intended behavior, so I had to make the `exclude` in the `Cargo.toml` more specific to make sure this doesn't happen (I tried using a glob, but that didn't work, apparently because of rust-lang/cargo#6009. Resolves #2848 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

`rustc_span` cleanups Just some things I found while looking over this crate. r? `@oli-obk`

rustbot assigned oli-obk Nov 2, 2023

rustbot added 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-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Nov 2, 2023

Noratrieb reviewed Nov 2, 2023

View reviewed changes

compiler/rustc_span/src/lib.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Noratrieb reviewed Nov 2, 2023

View reviewed changes

compiler/rustc_span/src/fatal_error.rs Show resolved Hide resolved

nnethercote added 5 commits November 2, 2023 17:07

Explain the () argument to ErrorGuaranteed.

d76661b

Fix some comments.

1bcb600

The comment just below the first one describes how the `impl !Send for FatalError` makes it impossible to `panic!(FatalError)`. And the second one should be `panic_any` instead of `panic!`.

Tweak use items.

16e9713

Minimize pub usage in hygiene.rs.

222c223

And remove dead functions revealed by this.

Formatting tweaks.

84773b3

nnethercote force-pushed the rustc_span branch from 36a3abd to b390737 Compare November 2, 2023 06:23

rustbot assigned Noratrieb and unassigned oli-obk Nov 2, 2023

This comment has been minimized.

Sign in to view

nnethercote added 4 commits November 2, 2023 19:35

Deinline all session global functions.

edb357f

These are all called very rarely, so there is no need for them to be inline.

Replace two create_default_session_if_not_set_then uses.

9893b75

With `create_default_session_globals_then`, which is preferable when it is appropriate.

Add a couple of clarifying comments.

6358411

nnethercote force-pushed the rustc_span branch from b390737 to 6358411 Compare November 2, 2023 08:35

xFrednet mentioned this pull request Nov 2, 2023

Bump Nightly 2023-11-16 rust-marker/marker#280

Closed

Noratrieb reviewed Nov 2, 2023

View reviewed changes

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 Nov 3, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 3, 2023

bors merged commit 9c20ddd into rust-lang:master Nov 3, 2023
12 checks passed

rustbot added this to the 1.75.0 milestone Nov 3, 2023

bors mentioned this pull request Nov 3, 2023

Do not insert duplicate SourceFile. #115572

Closed

nnethercote deleted the rustc_span branch November 3, 2023 22:03

github-actions bot mentioned this pull request Nov 6, 2023

Automated pull from upstream master ferrocene/ferrocene#84

Merged

zhassan-aws mentioned this pull request Nov 6, 2023

Update Rust toolchain to 2023-11-06 model-checking/kani#2858

Merged

flip1995 pushed a commit to flip1995/rust that referenced this pull request Nov 16, 2023

Auto merge of rust-lang#117507 - nnethercote:rustc_span, r=Nilstrieb

2d9af16

`rustc_span` cleanups Just some things I found while looking over this crate. r? `@oli-obk`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`rustc_span` cleanups #117507

`rustc_span` cleanups #117507

nnethercote commented Nov 2, 2023

rustbot commented Nov 2, 2023

This comment has been minimized.

nnethercote commented Nov 2, 2023

This comment has been minimized.

Noratrieb Nov 2, 2023

nnethercote Nov 2, 2023

nnethercote Nov 2, 2023

Noratrieb Nov 3, 2023

Noratrieb left a comment

Noratrieb commented Nov 3, 2023

bors commented Nov 3, 2023

bors commented Nov 3, 2023

bors commented Nov 3, 2023

rust-timer commented Nov 3, 2023

rustc_span cleanups #117507

rustc_span cleanups #117507

Conversation

nnethercote commented Nov 2, 2023

rustbot commented Nov 2, 2023

This comment has been minimized.

nnethercote commented Nov 2, 2023

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Noratrieb left a comment

Choose a reason for hiding this comment

Noratrieb commented Nov 3, 2023

bors commented Nov 3, 2023

bors commented Nov 3, 2023

bors commented Nov 3, 2023

rust-timer commented Nov 3, 2023

Overall result: ❌ regressions - no action needed

`rustc_span` cleanups #117507

`rustc_span` cleanups #117507