delete [allow(unused_unsafe)] from issue #74838

[mj10021]

While looking into issue #111288 I noticed the following #[allow(...)] with a FIXME asking for it to be removed. Deleting the #[allow(...)] does not seem to break anything, it seems like the lint has been updated for unsafe blocks in macros?

[rustbot]

r? @joshtriplett

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[jyn514]

it would be nice to track down when this changed so we know whether or not to close #111288. do you have time to do that?

[mj10021]

it would be nice to track down when this changed so we know whether or not to close #111288. do you have time to do that?

Do you mean to track down when the lint from #111288 regressed or when the [allow(...)] stopped being necessary? This doesn't fix #111288 and I actually think #111288 is a bigger issue regarding warnings from macros in general

[jyn514]

When the allow stopped being necessary. I'm trying to find out if #111288 is still an issue or not. Another (maybe easier?) way would be to write a small program that shouldn't lint about unsafe but does and that would confirm it's still an issue.

[mj10021]

Ok gotcha I will look into it

[mj10021]

@jyn514 it looks like commit e1d7dec558d is the one that made the allow unnecessary, does that make any sense?

[jyn514]

@mj10021 it makes sense, but it doesn't actually help narrow this down much - that commit includes all the changes between 1.64 beta and 1.65 beta, so the change in the lint could have happened any time in those 6 weeks.

[jyn514]

Sorry, I've been confused this whole time. I meant to say which change made 74838 not warn any more, not 111288.

[mj10021]

@mj10021 it makes sense, but it doesn't actually help narrow this down much - that commit includes all the changes between 1.64 beta and 1.65 beta, so the change in the lint could have happened any time in those 6 weeks.

ah ok gotcha I will try and narrow it down, and np I kind of assumed you meant #74838

[mj10021]

I got the following from cargo-bisect-rustc:

********************************************************************************
Regression in nightly-2022-08-20
********************************************************************************

fetching https://static.rust-lang.org/dist/2022-08-19/channel-rust-nightly-git-commit-hash.txt
nightly manifest 2022-08-19: 40 B / 40 B [======================] 100.00 % 776.59 KB/s converted 2022-08-19 to 0b79f758c9aa6646606662a6d623a0752286cd17
fetching https://static.rust-lang.org/dist/2022-08-20/channel-rust-nightly-git-commit-hash.txt
nightly manifest 2022-08-20: 40 B / 40 B [======================] 100.00 % 503.39 KB/s converted 2022-08-20 to e1b28cd2f16bd5b832183d7968cae3bb9213e78d
looking for regression commit between 2022-08-19 and 2022-08-20
cloning rust repository
fetching (via local git) commits from 0b79f758c9aa6646606662a6d623a0752286cd17 to e1b28cd2f16bd5b832183d7968cae3bb9213e78d
opening existing repository at "rust.git"
Found origin remote under name `origin`
refreshing repository at "rust.git"
looking up first commit
looking up second commit
checking that commits are by bors and thus have ci artifacts...
finding bors merge commits
found 5 bors merge commits in the specified range
  commit[0] 2022-08-18: Auto merge of #98807 - cbeuw:derived-obligation, r=compiler-errors
  commit[1] 2022-08-18: Auto merge of #98851 - klensy:encode_symbols, r=cjgillot
  commit[2] 2022-08-19: Auto merge of #99541 - timvermeulen:flatten_cleanup, r=the8472
  commit[3] 2022-08-19: Auto merge of #100209 - cjgillot:source-file-index, r=estebank
  commit[4] 2022-08-19: Auto merge of #100740 - Dylan-DPC:rollup-0td6yq4, r=Dylan-DPC
ERROR: no CI builds available between 0b79f758c9aa6646606662a6d623a0752286cd17 and e1b28cd2f16bd5b832183d7968cae3bb9213e78d within last 167 days

It's not obvious to me why any of the commits affect the unused_unsafe in macros...

[mj10021]

Nevermind! I think it's this: #100081

[mj10021]

hey @jyn514 just following up here, is there anything else that I should do for this pr?

[jyn514]

r? libs

[cuviper]

There are a few more allows around thread locals -- can they all be removed?

rust/library/std/src/sys/common/thread_local/os_local.rs

Line 27 in 114fb86

#[allow(unused_unsafe)]

rust/library/std/src/sys/common/thread_local/os_local.rs

Lines 62 to 65 in 114fb86

	// FIXME: remove the #[allow(...)] marker when macros don't
	// raise warning for missing/extraneous unsafe blocks anymore.
	// See https://github.com/rust-lang/rust/issues/74838.
	#[allow(unused_unsafe)]

rust/library/std/src/sys/common/thread_local/static_local.rs

Lines 46 to 49 in 114fb86

	// FIXME: remove the #[allow(...)] marker when macros don't
	// raise warning for missing/extraneous unsafe blocks anymore.
	// See https://github.com/rust-lang/rust/issues/74838.
	#[allow(unused_unsafe)]

rust/library/std/src/sys_common/thread_info.rs

Line 2 in 114fb86

#![allow(unused_unsafe)] // thread_local with `const {}` triggers this liny

[mj10021]

ah good point, I took out all of the allow(unused_unsafe)s in /library/std and it seems to be building fine

[cuviper]

Thanks!

@bors r+

[bors]

📌 Commit db43036424e60025f26a90c1cb94e97fcd6fe720 has been approved by cuviper

It is now in the queue for this repository.

[bors]

⌛ Testing commit db43036424e60025f26a90c1cb94e97fcd6fe720 with merge aea349ee63c905a9f24e2364eecd9968245e7f31...

[bors]

💔 Test failed - checks-actions

[mj10021]

I am assuming the #[allow... that needed to be added back was in library/std/src/sys/unix/weak.rs... do you know how I can rerun the arm-android test?

[cuviper]

You can add temporarily add it to the pr job here, and re-run the command mentioned at the top of that file.

[mj10021]

You can add temporarily add it to the pr job here, and re-run the command mentioned at the top of that file.

since it passes the CI should i just remove the last commit and push again?

[cuviper]

Yeah, and can you also squash the 3rd commit, so we don't have commits removing and adding the same line?

In case you don't know how, git rebase -i should be straightforward -- just mark the 3rd commit as a fixup and delete the 4th commit altogether.

[mj10021]

Yeah, and can you also squash the 3rd commit, so we don't have commits removing and adding the same line?

In case you don't know how, git rebase -i should be straightforward -- just mark the 3rd commit as a fixup and delete the 4th commit altogether.

yeah np on it rn

[cuviper]

OK, let's try it!

@bors r+

[bors]

📌 Commit db4a153 has been approved by cuviper

It is now in the queue for this repository.

[bors]

⌛ Testing commit db4a153 with merge 1821920...

[bors]

☀️ Test successful - checks-actions
Approved by: cuviper
Pushing 1821920 to master...

[rust-timer]

Finished benchmarking commit (1821920): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.4%	[0.4%, 0.4%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.6%]	4
Improvements ✅ (secondary)	-0.5%	[-0.5%, -0.3%]	5
All ❌✅ (primary)	-0.5%	[-0.7%, 0.4%]	5

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)	-	-	0
Improvements ✅ (primary)	-3.3%	[-3.3%, -3.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.3%	[-3.3%, -3.3%]	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: 650.065s -> 652.747s (0.41%)

[nnethercote]

One tiny regression; this is fine.

@rustbot label: +perf-regression-triaged