Display function path in unsafety violations - E0133 #96294

Emilgardis · 2022-04-21T19:06:25Z

adds DefId to UnsafetyViolationDetails

this enables consumers to access the function definition that was reported to be unsafe and also changes the output for some E0133 diagnostics

rust-highfive · 2022-04-21T19:06:28Z

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

Emilgardis · 2022-04-21T19:07:13Z

see (small) zulip discussion

estebank · 2022-04-22T00:24:10Z

r? @oli-obk

oli-obk · 2022-04-22T08:21:22Z

@bors try @rust-timer queue

rust-timer · 2022-04-22T08:21:24Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-04-22T08:21:32Z

⌛ Trying commit 434eb13 with merge 165df72f4556d54e1da1587e70ee8fe7448df49b...

compiler/rustc_middle/src/mir/query.rs

bors · 2022-04-22T09:45:21Z

☀️ Try build successful - checks-actions
Build commit: 165df72f4556d54e1da1587e70ee8fe7448df49b (165df72f4556d54e1da1587e70ee8fe7448df49b)

rust-timer · 2022-04-22T09:45:22Z

Queued 165df72f4556d54e1da1587e70ee8fe7448df49b with parent 1158ade, future comparison URL.

rust-timer · 2022-04-22T11:00:27Z

Finished benchmarking commit (165df72f4556d54e1da1587e70ee8fe7448df49b): comparison url.

Summary:

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: no relevant changes found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	1	0	0	0	1
mean²	1.8%	N/A	N/A	N/A	1.8%
max	1.8%	N/A	N/A	N/A	1.8%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf -perf-regression

number of relevant changes ↩
the arithmetic mean of the percent change ↩

oli-obk · 2022-04-23T07:05:49Z

The perf regression is just the usual syn noise

Emilgardis · 2022-04-23T14:52:37Z

compiler/rustc_middle/src/mir/query.rs

-                "call to unsafe function",
+            CallToUnsafeFunction(did) => (
+                if let Some(did) = did {
+                    Cow::from(format!("call to unsafe function `{}`", tcx.def_path_str(*did)))


I'm not entirely sure I like this wording

oli-obk · 2022-04-23T16:20:03Z

You need to update some more //~ ERROR annotations

Emilgardis · 2022-04-23T19:40:39Z

Done, I hope.

https://rustc-dev-guide.rust-lang.org/tests/ui.html#cfg-revisions needs to be updated also.

Was thinking about dropping the is unsafe part in the UI tests to make it match in both revisions, but I think this makes more sense.

also, if to be merged, the title should probably be changed

oli-obk · 2022-04-23T22:30:26Z

Ah interesting. The thir unsafety check is what we are moving to, so it may be that some day in the future you may lose all the information you're looking for.

I'm not sure we even have a way of extracting the information you want from THIR.

So for now, this will work, and we'll adjust the thir diagnostics to match, but I'm not sure how to give you what you need in the longer term.

Emilgardis · 2022-04-24T12:44:49Z

fixed the inconsistency between thir and mir, with the added benefit that thir will display a nicer path.

Emilgardis · 2022-04-24T12:49:44Z

src/test/ui/threads-sendsync/issue-43733.rs

+    //[mir]~^ ERROR call to unsafe function `std::thread::__FastLocalKeyInner::<T>::get` is unsafe
+    //[thir]~^^ ERROR call to unsafe function `__FastLocalKeyInner::<T>::get` is unsafe


this is the difference between mir and thir now.

sidenote: I don't understand why def_path_str decides to remove std::thread here

That usually happens when there are imports for these modules. Anyway, seems fine to me for now.

Emilgardis · 2022-04-24T22:27:28Z

I don't know how to solve this but to remove stderr checking

Emilgardis · 2022-04-25T09:47:49Z

fixed with some normalization :D

oli-obk · 2022-04-25T14:31:32Z

Ah perfect, you found it!

@bors r+

bors · 2022-04-25T14:31:33Z

📌 Commit f71597c has been approved by oli-obk

bors · 2022-04-25T23:03:55Z

⌛ Testing commit f71597c with merge ec8619d...

bors · 2022-04-26T01:52:41Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing ec8619d to master...

rust-timer · 2022-04-26T03:08:31Z

Finished benchmarking commit (ec8619d): comparison url.

Summary:

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: no relevant changes found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	44	0	0	0	44
mean²	1.3%	N/A	N/A	N/A	1.3%
max	2.9%	N/A	N/A	N/A	2.9%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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

number of relevant changes ↩
the arithmetic mean of the percent change ↩

oli-obk · 2022-04-26T07:18:49Z

Oh oh, that perf change was unexpected. It's solely in incremental, rereading the source to find out what happened

oli-obk · 2022-04-26T08:58:41Z

@Emilgardis do you still need the def ids in the result of unsafety_check_result? You mentioned working on the THIR now, if that's the case, I'll revert the changes outside of THIR. We'll get the better diagnostics again once we fully move to THIR.

rylev · 2022-04-26T09:02:50Z

@oli-obk @Emilgardis Do you expect to be able to address the perf regressions here? It seems they were introduced by a code that's not fundamental to this change since previous perf runs didn't report these regressions. If you're not able to get to it soon, can you please open an issue for this so that it doesn't get lost? Once you do that, we can mark this as triaged.

oli-obk · 2022-04-26T09:03:59Z

Yes, I just need feedback to see if the easy solution can be done first, if not, i'll look into alternative routes.

Emilgardis · 2022-04-26T10:01:42Z

@Emilgardis do you still need the def ids in the result of unsafety_check_result? You mentioned working on the THIR now, if that's the case, I'll revert the changes outside of THIR. We'll get the better diagnostics again once we fully move to THIR.

Go ahead, not needed

oli-obk · 2022-04-26T10:45:28Z

@rustbot label: +perf-regression-triaged

#96425 should address the perf regression

…safety_checking, r=compiler-errors Fix incremental perf regression unsafety checking Perf regression introduced in rust-lang#96294 We will simply avoid emitting the name of the unsafe function in MIR unsafeck, since we're moving to THIR unsafeck anyway.

rust-highfive assigned estebank Apr 21, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 21, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 21, 2022

rust-highfive assigned oli-obk and unassigned estebank Apr 22, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 22, 2022

oli-obk reviewed Apr 22, 2022

View reviewed changes

compiler/rustc_middle/src/mir/query.rs Outdated Show resolved Hide resolved

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 22, 2022

Emilgardis commented Apr 23, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

Emilgardis force-pushed the def_id-in-unsafetyviolationdetails branch from 2c3c140 to 5644d19 Compare April 23, 2022 19:39

Emilgardis force-pushed the def_id-in-unsafetyviolationdetails branch from 5644d19 to aeab071 Compare April 24, 2022 12:43

Emilgardis commented Apr 24, 2022

View reviewed changes

Emilgardis changed the title ~~add DefId to UnsafetyViolationDetails~~ add DefId to unsafety violations and display function path in E0133 Apr 24, 2022

Emilgardis changed the title ~~add DefId to unsafety violations and display function path in E0133~~ Display function path in unsafety violations - E0133 Apr 24, 2022

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 24, 2022

This comment has been minimized.

Sign in to view

avoid fully qualifying error output of issue-43733 test

f71597c

Emilgardis force-pushed the def_id-in-unsafetyviolationdetails branch from d0be897 to f71597c Compare April 25, 2022 09:30

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 Apr 25, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 26, 2022

bors merged commit ec8619d into rust-lang:master Apr 26, 2022

rustbot added this to the 1.62.0 milestone Apr 26, 2022

rustbot added the perf-regression Performance regression. label Apr 26, 2022

oli-obk mentioned this pull request Apr 26, 2022

Fix incremental perf regression unsafety checking #96425

Merged

rustbot added the perf-regression-triaged The performance regression has been triaged. label Apr 26, 2022

Emilgardis deleted the def_id-in-unsafetyviolationdetails branch April 26, 2022 17:14

LeSeulArtichaut mentioned this pull request Dec 24, 2022

Don't display function path for unsafe_op_in_unsafe_fn lints #106127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display function path in unsafety violations - E0133 #96294

Display function path in unsafety violations - E0133 #96294

Emilgardis commented Apr 21, 2022 •

edited

Loading

rust-highfive commented Apr 21, 2022

Emilgardis commented Apr 21, 2022

estebank commented Apr 22, 2022

oli-obk commented Apr 22, 2022

rust-timer commented Apr 22, 2022

bors commented Apr 22, 2022

bors commented Apr 22, 2022

rust-timer commented Apr 22, 2022

rust-timer commented Apr 22, 2022

oli-obk commented Apr 23, 2022

Emilgardis Apr 23, 2022

This comment has been minimized.

oli-obk commented Apr 23, 2022

Emilgardis commented Apr 23, 2022 •

edited

Loading

oli-obk commented Apr 23, 2022

Emilgardis commented Apr 24, 2022

Emilgardis Apr 24, 2022

oli-obk Apr 24, 2022

Emilgardis commented Apr 24, 2022

This comment has been minimized.

Emilgardis commented Apr 25, 2022

oli-obk commented Apr 25, 2022

bors commented Apr 25, 2022

bors commented Apr 25, 2022

bors commented Apr 26, 2022

rust-timer commented Apr 26, 2022

oli-obk commented Apr 26, 2022

oli-obk commented Apr 26, 2022

rylev commented Apr 26, 2022

oli-obk commented Apr 26, 2022

Emilgardis commented Apr 26, 2022

oli-obk commented Apr 26, 2022

		//[mir]~^ ERROR call to unsafe function `std::thread::__FastLocalKeyInner::<T>::get` is unsafe
		//[thir]~^^ ERROR call to unsafe function `__FastLocalKeyInner::<T>::get` is unsafe

Display function path in unsafety violations - E0133 #96294

Display function path in unsafety violations - E0133 #96294

Conversation

Emilgardis commented Apr 21, 2022 • edited Loading

rust-highfive commented Apr 21, 2022

Emilgardis commented Apr 21, 2022

estebank commented Apr 22, 2022

oli-obk commented Apr 22, 2022

rust-timer commented Apr 22, 2022

bors commented Apr 22, 2022

bors commented Apr 22, 2022

rust-timer commented Apr 22, 2022

rust-timer commented Apr 22, 2022

Footnotes

oli-obk commented Apr 23, 2022

Emilgardis Apr 23, 2022

Choose a reason for hiding this comment

This comment has been minimized.

oli-obk commented Apr 23, 2022

Emilgardis commented Apr 23, 2022 • edited Loading

oli-obk commented Apr 23, 2022

Emilgardis commented Apr 24, 2022

Emilgardis Apr 24, 2022

Choose a reason for hiding this comment

oli-obk Apr 24, 2022

Choose a reason for hiding this comment

Emilgardis commented Apr 24, 2022

This comment has been minimized.

Emilgardis commented Apr 25, 2022

oli-obk commented Apr 25, 2022

bors commented Apr 25, 2022

bors commented Apr 25, 2022

bors commented Apr 26, 2022

rust-timer commented Apr 26, 2022

Footnotes

oli-obk commented Apr 26, 2022

oli-obk commented Apr 26, 2022

rylev commented Apr 26, 2022

oli-obk commented Apr 26, 2022

Emilgardis commented Apr 26, 2022

oli-obk commented Apr 26, 2022

Emilgardis commented Apr 21, 2022 •

edited

Loading

Emilgardis commented Apr 23, 2022 •

edited

Loading