Suggest using block for `extern "abi" fn` with no body #98827

aDotInTheVoid · 2022-07-02T22:34:20Z

@rustbot modify labels: +A-diagnostics

rustbot · 2022-07-02T22:34:23Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

rust-highfive · 2022-07-02T22:34:23Z

r? @nagisa

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

nagisa

Lets run a perfbot and see if spans in extern are reasonably cheap addition.

nagisa · 2022-07-03T22:54:03Z

src/test/ui/extern/not-in-block.stderr

+error: `extern` functions without bodies must go in `extern` blocks
+  --> $DIR/not-in-block.rs:3:1
+   |
+LL | extern fn none_fn(x: bool) -> i32;
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: wrap in an `extern` block
+   |
+LL | extern { fn none_fn(x: bool) -> i32; }
+   | ~~~~~~~~                             +


Wording suggestion: I would largely follow the messaging of non-extern functions for the error message, only adapting a help message to be specific to extern. Among other reasons to do so is just plain sharing of the effort on this diagnostic between the two types of functions. Today that reads as:

error: free function without a body --> src/lib.rs:1:1 | 1 | fn banana(); | ^^^^^^^^^^^- | | | help: provide a definition for the function: `{ <body> }`

Regardless of whether this item has an extern or not, it is pretty hard to tell what the intent really was. Did they mean to declare something? Did they make a genuine mistake? The current wording does a great job at not assuming a specific problem. Another thing I’d like to see improved is explaining what is the effect of wrapping the item in an extern block. That way somebody seeing the message for the first time won’t need to go searching for the documentation describing what and why. With that in mind… maybe something like this?

Suggested change

error: `extern` functions without bodies must go in `extern` blocks

--> $DIR/not-in-block.rs:3:1

|

LL | extern fn none_fn(x: bool) -> i32;

| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

|

help: wrap in an `extern` block

|

LL | extern { fn none_fn(x: bool) -> i32; }

| ~~~~~~~~ +

error: free function without a body

--> $DIR/not-in-block.rs:3:1

|

LL | extern fn none_fn(x: bool) -> i32;

| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

|

help: use an `extern` block in order to declare an externally defined function

|

LL | extern { fn none_fn(x: bool) -> i32; }

| ~~~~~~~~ +

nagisa · 2022-07-03T23:07:52Z

@bors try @rust-timer queue

rust-timer · 2022-07-03T23:07:53Z

Awaiting bors try build completion.

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

bors · 2022-07-03T23:08:00Z

⌛ Trying commit 6ff30d72cdfe6fb0b5a9db25e4380f0bde5411af with merge bcd60280e79a3ae4298f75261dee911e1466f6be...

bors · 2022-07-04T00:58:26Z

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

rust-timer · 2022-07-04T00:58:28Z

Queued bcd60280e79a3ae4298f75261dee911e1466f6be with parent 495b216, future comparison URL.

rust-timer · 2022-07-04T02:14:21Z

Finished benchmarking commit (bcd60280e79a3ae4298f75261dee911e1466f6be): comparison url.

Instruction count

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	0.3%	0.3%	6
Regressions 😿 (secondary)	0.5%	0.6%	7
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-0.4%	-0.5%	4
All 😿🎉 (primary)	0.3%	0.3%	6

Max RSS (memory usage)

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

Cycles

Results

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-3.1%	-4.7%	2
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-3.1%	-4.7%	2

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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

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

aDotInTheVoid · 2022-07-05T01:20:53Z

Updated the diagnostic messages. Perf result is unfortunate, not sure if it's Ok to land like this, if it's possible to get the better errors without the perf overhead, or how to decide if it's worth it.

@rustbot ready

nagisa · 2022-07-05T08:39:26Z

@nnethercote I wonder what your take is here. Do you think there might be a better place to keep the Span for extern here?

compiler/rustc_ast_passes/src/ast_validation.rs

nnethercote · 2022-07-06T10:50:46Z

The performance regressions are very small and on only a couple of benchmarks. I wouldn't worry about it.

rust-log-analyzer · 2022-07-06T13:13:30Z

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

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

  IMAGE: x86_64-gnu-tools
##[endgroup]
From https://github.com/rust-lang/rust
 * branch              master     -> FETCH_HEAD
Searching for toolstate changes between 5b8cf49c51833ee5d27ae2e8e179337dbb9f14d7 and ca3c07560e6da1adf890ddd838208bad545d6fe2
Clippy or rustfmt subtrees were updated
##[group]Run src/ci/scripts/verify-channel.sh
src/ci/scripts/verify-channel.sh
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
---
.......... (60/66)
.....     (66/66)


/checkout/src/test/rustdoc-gui/search-filter.goml search-filter... FAILED
[ERROR] (line 6) Error: The following CSS selector "#titles" was not found: for command `wait-for: "#titles"`
Build completed unsuccessfully in 0:00:45

aDotInTheVoid · 2022-07-06T15:10:17Z

@bors retry

bors · 2022-07-06T15:10:18Z

@aDotInTheVoid: 🔑 Insufficient privileges: not in try users

nagisa · 2022-07-06T20:46:33Z

@bors r+

bors · 2022-07-06T20:46:35Z

📌 Commit 02fb345 has been approved by nagisa

bors · 2022-07-07T04:18:50Z

⌛ Testing commit 02fb345 with merge e78e747...

bors · 2022-07-07T06:43:21Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing e78e747 to master...

rust-timer · 2022-07-07T16:58:58Z

Finished benchmarking commit (e78e747): comparison url.

Instruction count

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-0.8%	-0.9%	6
Improvements 🎉 (secondary)	-1.6%	-2.3%	9
All 😿🎉 (primary)	-0.8%	-0.9%	6

Max RSS (memory usage)

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: 🎉 relevant improvement found

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

Cycles

Results

Primary benchmarks: 🎉 relevant improvement found
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-2.3%	-2.3%	1
Improvements 🎉 (secondary)	-2.9%	-3.7%	8
All 😿🎉 (primary)	-2.3%	-2.3%	1

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

@rustbot label: -perf-regression

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

nnethercote · 2022-07-07T22:00:15Z

I'm pretty sure those perf results are just noise, based on (a) how they differ to the pre-merge CI perf run, and (b) similar kinds of results in other PRs lately.

…=nagisa Suggest using block for `extern "abi" fn` with no body `@rustbot` modify labels: +A-diagnostics

ast: Add span to Extern

18ca294

rust-highfive assigned nagisa Jul 2, 2022

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

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

rustbot added the A-diagnostics Area: Messages for errors, warnings, and lints label Jul 2, 2022

nagisa reviewed Jul 3, 2022

View reviewed changes

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

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 4, 2022

aDotInTheVoid force-pushed the suggest-extern-block branch from 6ff30d7 to 50e5059 Compare July 5, 2022 01:17

nnethercote reviewed Jul 6, 2022

View reviewed changes

compiler/rustc_ast_passes/src/ast_validation.rs Outdated Show resolved Hide resolved

aDotInTheVoid force-pushed the suggest-extern-block branch from d016d08 to e83554b Compare July 6, 2022 12:01

This comment has been minimized.

Sign in to view

Suggest using block for extern "abi" fn with no body

02fb345

aDotInTheVoid force-pushed the suggest-extern-block branch from e83554b to 02fb345 Compare July 6, 2022 12:28

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 6, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 7, 2022

bors merged commit e78e747 into rust-lang:master Jul 7, 2022

rustbot added this to the 1.64.0 milestone Jul 7, 2022

rustbot removed the perf-regression Performance regression. label Jul 7, 2022

flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 18, 2022

Auto merge of rust-lang#98827 - aDotInTheVoid:suggest-extern-block, r…

5483a7d

…=nagisa Suggest using block for `extern "abi" fn` with no body `@rustbot` modify labels: +A-diagnostics

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggest using block for `extern "abi" fn` with no body #98827

Suggest using block for `extern "abi" fn` with no body #98827

aDotInTheVoid commented Jul 2, 2022

rustbot commented Jul 2, 2022

rust-highfive commented Jul 2, 2022

nagisa left a comment

nagisa Jul 3, 2022 •

edited

Loading

nagisa commented Jul 3, 2022

rust-timer commented Jul 3, 2022

bors commented Jul 3, 2022

bors commented Jul 4, 2022

rust-timer commented Jul 4, 2022

rust-timer commented Jul 4, 2022

aDotInTheVoid commented Jul 5, 2022 •

edited

Loading

nagisa commented Jul 5, 2022

nnethercote commented Jul 6, 2022

This comment has been minimized.

rust-log-analyzer commented Jul 6, 2022

aDotInTheVoid commented Jul 6, 2022

bors commented Jul 6, 2022

nagisa commented Jul 6, 2022

bors commented Jul 6, 2022

bors commented Jul 7, 2022

bors commented Jul 7, 2022

rust-timer commented Jul 7, 2022

nnethercote commented Jul 7, 2022

Suggest using block for extern "abi" fn with no body #98827

Suggest using block for extern "abi" fn with no body #98827

Conversation

aDotInTheVoid commented Jul 2, 2022

rustbot commented Jul 2, 2022

rust-highfive commented Jul 2, 2022

nagisa left a comment

Choose a reason for hiding this comment

nagisa Jul 3, 2022 • edited Loading

Choose a reason for hiding this comment

nagisa commented Jul 3, 2022

rust-timer commented Jul 3, 2022

bors commented Jul 3, 2022

bors commented Jul 4, 2022

rust-timer commented Jul 4, 2022

rust-timer commented Jul 4, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

aDotInTheVoid commented Jul 5, 2022 • edited Loading

nagisa commented Jul 5, 2022

nnethercote commented Jul 6, 2022

This comment has been minimized.

rust-log-analyzer commented Jul 6, 2022

aDotInTheVoid commented Jul 6, 2022

bors commented Jul 6, 2022

nagisa commented Jul 6, 2022

bors commented Jul 6, 2022

bors commented Jul 7, 2022

bors commented Jul 7, 2022

rust-timer commented Jul 7, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

nnethercote commented Jul 7, 2022

Suggest using block for `extern "abi" fn` with no body #98827

Suggest using block for `extern "abi" fn` with no body #98827

nagisa Jul 3, 2022 •

edited

Loading

aDotInTheVoid commented Jul 5, 2022 •

edited

Loading