Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change return type of unstable Waker::noop() from Waker to &Waker. #119984

Merged
merged 2 commits into from
Jan 20, 2024

Conversation

kpreid
Copy link
Contributor

@kpreid kpreid commented Jan 15, 2024

The advantage of this is that it does not need to be assigned to a variable to be used in a Context creation, which is the most common thing to want to do with a noop waker. It also avoids unnecessarily executing the dynamically dispatched drop function when the noop waker is dropped.

If an owned noop waker is desired, it can be created by cloning, but the reverse is harder to do since it requires declaring a constant. Alternatively, both versions could be provided, like futures::task::noop_waker() and futures::task::noop_waker_ref(), but that seems to me to be API clutter for a very small benefit, whereas having the &'static reference available is a large reduction in boilerplate.

Previous discussion on the tracking issue starting here

@rustbot
Copy link
Collaborator

rustbot commented Jan 15, 2024

r? @TaKO8Ki

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

@rustbot 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-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 15, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 15, 2024

The Miri subtree was changed

cc @rust-lang/miri

@rust-lang rust-lang deleted a comment from rustbot Jan 15, 2024
@RossSmyth
Copy link
Contributor

If you are poking this I would bring up one comment from the previous impl that was not really addressed (understandably since it was like a year later).

Maybe make this a static such that the vtable is shared between all times RawWaker is codegened?

Instead of consts, using statics for the RawWaker and probably the Waker itself since it's a ref now may be a good idea so that they are not duplicated each time (which I think is how it would work as implemented now). This may slightly reduce compile times if used multiple times. Only downside I can think of is it would then be easily inspected to be a noop, if that even matters.

@traviscross traviscross added the WG-async Working group: Async & await label Jan 15, 2024
@compiler-errors
Copy link
Member

r? libs-api

IDK why this was assigned to a compiler contributor.

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 15, 2024
@rustbot rustbot assigned dtolnay and unassigned TaKO8Ki Jan 15, 2024
@kpreid
Copy link
Contributor Author

kpreid commented Jan 15, 2024

using statics for the RawWaker and probably the Waker itself

I'd love to do that, but unfortunately, that's prohibited unless #119618 happens:

error[E0013]: constant functions cannot refer to statics
   --> library/core/src/task/wake.rs:371:10
    |
371 |         &WAKER
    |          ^^^^^
    |
    = help: consider extracting the value of the `static` to a `const`, and referring to that

A possible alternative would be a pub static NOOP: Waker, but "error: associated static items are not allowed" either, so then it would have to be a non-associated static item, which doesn't seem worth it to me.

IDK why this was assigned to a compiler contributor.

Perhaps because I modified tests/ui/?

@rustbot label -T-compiler

@rustbot rustbot removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 15, 2024
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@dtolnay
Copy link
Member

dtolnay commented Jan 15, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jan 15, 2024

📌 Commit 7052188 has been approved by dtolnay

It is now in the queue for this repository.

@bors 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 Jan 15, 2024
Comment on lines 18 to 19
$value.or_else(|e| {
// FIXME(85000): no coverage in closure macros
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: moving this comment (presumably due to rustfmt?) is what caused closure_macro_async.cov-map to churn and need re-blessing. Moving it back (and blessing again) would make the diff simpler.

However, now that this PR has been approved and is in the queue, feel free to not worry about it. It's not a big deal either way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(There's no actual problem with moving the comment, and these things occasionally churn for all sorts of other reasons anyway.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack (both senses). I tried to revert all the unrelated rustfmt changes, but I missed this one because it was comparatively short. I've fixed this and will push an updated version after running local tests and we'll see what wins.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would prefer to keep what's in the PR. Reverting that rustfmt-generated change will just cause whoever is the next person to touch this file to stumble on the same thing again.

Alternatively, I would be open to reverting the rustfmt-generated change from this PR and then receiving a separate PR that applies rustfmt on all the files under tests/coverage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy either way. If this goes through as-is, I won't even bother reverting the change, because in this case the non-standard formatting isn't load-bearing anyway.

There are some other tests/coverage tests where non-standard formatting is load-bearing (because certain things need to be split across multiple lines); I should probably go through and mark those explicitly with no-format attributes at some point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Is there a standard way to tell x.py to format tests that normally wouldn't be touched by x fmt, or are people just running rustfmt by hand?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is relevant, but what happened in this PR is that rust-analyzer ran rustfmt for me on the files I edited.

I think I would prefer to keep what's in the PR.

Ack. Will not push.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 16, 2024
Change return type of unstable `Waker::noop()` from `Waker` to `&Waker`.

The advantage of this is that it does not need to be assigned to a variable to be used in a `Context` creation, which is the most common thing to want to do with a noop waker. It also avoids unnecessarily executing the dynamically dispatched drop function when the noop waker is dropped.

If an owned noop waker is desired, it can be created by cloning, but the reverse is harder to do since it requires declaring a constant. Alternatively, both versions could be provided, like `futures::task::noop_waker()` and `futures::task::noop_waker_ref()`, but that seems to me to be API clutter for a very small benefit, whereas having the `&'static` reference available is a large reduction in boilerplate.

[Previous discussion on the tracking issue starting here](rust-lang#98286 (comment))
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#119062 (Deny braced macro invocations in let-else)
 - rust-lang#119922 (Rework how diagnostic lints are stored.)
 - rust-lang#119978 (Move async closure parameters into the resultant closure's future eagerly)
 - rust-lang#119984 (Change return type of unstable `Waker::noop()` from `Waker` to `&Waker`.)
 - rust-lang#120020 (Gracefully handle missing typeck information if typeck errored)
 - rust-lang#120021 (don't store const var origins for known vars)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 16, 2024
Change return type of unstable `Waker::noop()` from `Waker` to `&Waker`.

The advantage of this is that it does not need to be assigned to a variable to be used in a `Context` creation, which is the most common thing to want to do with a noop waker. It also avoids unnecessarily executing the dynamically dispatched drop function when the noop waker is dropped.

If an owned noop waker is desired, it can be created by cloning, but the reverse is harder to do since it requires declaring a constant. Alternatively, both versions could be provided, like `futures::task::noop_waker()` and `futures::task::noop_waker_ref()`, but that seems to me to be API clutter for a very small benefit, whereas having the `&'static` reference available is a large reduction in boilerplate.

[Previous discussion on the tracking issue starting here](rust-lang#98286 (comment))
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 17, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#119855 (Enable Static Builds for FreeBSD)
 - rust-lang#119922 (Rework how diagnostic lints are stored.)
 - rust-lang#119978 (Move async closure parameters into the resultant closure's future eagerly)
 - rust-lang#119984 (Change return type of unstable `Waker::noop()` from `Waker` to `&Waker`.)
 - rust-lang#120020 (Gracefully handle missing typeck information if typeck errored)
 - rust-lang#120021 (don't store const var origins for known vars)
 - rust-lang#120032 (Fix `rustc_abi` build on stable)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 17, 2024
@kpreid
Copy link
Contributor Author

kpreid commented Jan 17, 2024

Oh, I understand. My local branch had the version where I had no formatting change.

Considering the non-triviality of getting this right, I'm going to continue with that plan, and keep formatting changes out of this PR, despite the previous discussion. #120015 can address that.

The advantage of this is that it does not need to be assigned to a
variable to be used in a `Context` creation, which is the most common
thing to want to do with a noop waker.

If an owned noop waker is desired, it can be created by cloning, but the
reverse is harder. Alternatively, both versions could be provided, like
`futures::task::noop_waker()` and `futures::task::noop_waker_ref()`, but
that seems to me to be API clutter for a very small benefit, whereas
having the `&'static` reference available is a large benefit.

Previous discussion on the tracking issue starting here:
rust-lang#98286 (comment)
`Waker::noop()` now returns a `&'static Waker` reference, so it can be
passed directly to `Context` creation with no temporary lifetime issue.
@kpreid
Copy link
Contributor Author

kpreid commented Jan 17, 2024

I have pushed the new "clean" version with no extraneous formatting. The original is still available if required.

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 17, 2024
@Zalathar
Copy link
Contributor

I ran x test coverage against these changes on my machine, and everything succeeded, so hopefully you won't have any nasty surprises in CI this time.

@kpreid
Copy link
Contributor Author

kpreid commented Jan 18, 2024

@matthiaskrgr The problem has been fixed and this PR is in need of an r+, if I understand correctly.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 19, 2024
Warn when not having a profiler runtime means that coverage tests won't be run/blessed

On a few occasions (e.g. rust-lang#118036, rust-lang#119984) people have been tripped up by the fact that half of the coverage test suite is skipped by default, because it `// needs-profiler-support` and the profiler runtime is not actually built in any of the default config profiles.

(This is made worse by the fact that it isn't enabled in any of the PR CI jobs either. So people think that they've successfully blessed the test suite, and then get a rude surprise when their merge only fails in the full CI job suite.)

This PR adds a simple warning to compiletest that should alert the user in some cases. It's not foolproof, but it should increase the chances of catching this problem earlier in the PR process.
@the8472
Copy link
Member

the8472 commented Jan 19, 2024

@bors r=dtolnay

@bors
Copy link
Contributor

bors commented Jan 19, 2024

📌 Commit c48cdfe has been approved by dtolnay

It is now in the queue for this repository.

@bors 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 Jan 19, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 19, 2024
Rollup merge of rust-lang#120083 - Zalathar:no-profiler, r=wesleywiser

Warn when not having a profiler runtime means that coverage tests won't be run/blessed

On a few occasions (e.g. rust-lang#118036, rust-lang#119984) people have been tripped up by the fact that half of the coverage test suite is skipped by default, because it `// needs-profiler-support` and the profiler runtime is not actually built in any of the default config profiles.

(This is made worse by the fact that it isn't enabled in any of the PR CI jobs either. So people think that they've successfully blessed the test suite, and then get a rude surprise when their merge only fails in the full CI job suite.)

This PR adds a simple warning to compiletest that should alert the user in some cases. It's not foolproof, but it should increase the chances of catching this problem earlier in the PR process.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 19, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#117561 (Stabilize `slice_first_last_chunk`)
 - rust-lang#117662 ([rustdoc] Allows links in headings)
 - rust-lang#119815 (Format sources into the error message when loading codegen backends)
 - rust-lang#119835 (Exhaustiveness: simplify empty pattern logic)
 - rust-lang#119984 (Change return type of unstable `Waker::noop()` from `Waker` to `&Waker`.)
 - rust-lang#120009 (never_patterns: typecheck never patterns)
 - rust-lang#120122 (Don't add needs-triage to A-diagnostics)
 - rust-lang#120126 (Suggest `.swap()` when encountering conflicting borrows from `mem::swap` on a slice)
 - rust-lang#120134 (Restrict access to the private field of newtype indexes)

Failed merges:

 - rust-lang#119968 (Remove unused/unnecessary features)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#117561 (Stabilize `slice_first_last_chunk`)
 - rust-lang#117662 ([rustdoc] Allows links in headings)
 - rust-lang#119815 (Format sources into the error message when loading codegen backends)
 - rust-lang#119835 (Exhaustiveness: simplify empty pattern logic)
 - rust-lang#119984 (Change return type of unstable `Waker::noop()` from `Waker` to `&Waker`.)
 - rust-lang#120009 (never_patterns: typecheck never patterns)
 - rust-lang#120122 (Don't add needs-triage to A-diagnostics)
 - rust-lang#120126 (Suggest `.swap()` when encountering conflicting borrows from `mem::swap` on a slice)
 - rust-lang#120134 (Restrict access to the private field of newtype indexes)

Failed merges:

 - rust-lang#119968 (Remove unused/unnecessary features)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 455382d into rust-lang:master Jan 20, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 20, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2024
Rollup merge of rust-lang#119984 - kpreid:waker-noop, r=dtolnay

Change return type of unstable `Waker::noop()` from `Waker` to `&Waker`.

The advantage of this is that it does not need to be assigned to a variable to be used in a `Context` creation, which is the most common thing to want to do with a noop waker. It also avoids unnecessarily executing the dynamically dispatched drop function when the noop waker is dropped.

If an owned noop waker is desired, it can be created by cloning, but the reverse is harder to do since it requires declaring a constant. Alternatively, both versions could be provided, like `futures::task::noop_waker()` and `futures::task::noop_waker_ref()`, but that seems to me to be API clutter for a very small benefit, whereas having the `&'static` reference available is a large reduction in boilerplate.

[Previous discussion on the tracking issue starting here](rust-lang#98286 (comment))
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 20, 2024
coverage: Format all coverage tests with `rustfmt`

As suggested by <rust-lang#119984 (comment)>.

Test files in `tests/` are normally ignored by `x fmt`, but sometimes those files end up being run through `rustfmt` anyway, either by `rust-analyzer` or by hand.

When that happens, it's annoying to have to manually revert formatting changes that are unrelated to the actual changes being made. So it's helpful for the tests in the repository to already have standard formatting beforehand.

However, there are several coverage tests that deliberately use non-standard formatting, so that line counts reveal more information about where code regions begin and end. In those cases, we can use `#[rustfmt::skip]` to prevent that code from being disturbed.

`@rustbot` label +A-code-coverage
@kpreid kpreid deleted the waker-noop branch January 20, 2024 16:37
Nadrieril added a commit to Nadrieril/rust that referenced this pull request Jan 21, 2024
coverage: Format all coverage tests with `rustfmt`

As suggested by <rust-lang#119984 (comment)>.

Test files in `tests/` are normally ignored by `x fmt`, but sometimes those files end up being run through `rustfmt` anyway, either by `rust-analyzer` or by hand.

When that happens, it's annoying to have to manually revert formatting changes that are unrelated to the actual changes being made. So it's helpful for the tests in the repository to already have standard formatting beforehand.

However, there are several coverage tests that deliberately use non-standard formatting, so that line counts reveal more information about where code regions begin and end. In those cases, we can use `#[rustfmt::skip]` to prevent that code from being disturbed.

``@rustbot`` label +A-code-coverage
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 21, 2024
Rollup merge of rust-lang#120015 - Zalathar:format, r=dtolnay

coverage: Format all coverage tests with `rustfmt`

As suggested by <rust-lang#119984 (comment)>.

Test files in `tests/` are normally ignored by `x fmt`, but sometimes those files end up being run through `rustfmt` anyway, either by `rust-analyzer` or by hand.

When that happens, it's annoying to have to manually revert formatting changes that are unrelated to the actual changes being made. So it's helpful for the tests in the repository to already have standard formatting beforehand.

However, there are several coverage tests that deliberately use non-standard formatting, so that line counts reveal more information about where code regions begin and end. In those cases, we can use `#[rustfmt::skip]` to prevent that code from being disturbed.

``@rustbot`` label +A-code-coverage
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Apr 7, 2024
coverage: Format all coverage tests with `rustfmt`

As suggested by <rust-lang/rust#119984 (comment)>.

Test files in `tests/` are normally ignored by `x fmt`, but sometimes those files end up being run through `rustfmt` anyway, either by `rust-analyzer` or by hand.

When that happens, it's annoying to have to manually revert formatting changes that are unrelated to the actual changes being made. So it's helpful for the tests in the repository to already have standard formatting beforehand.

However, there are several coverage tests that deliberately use non-standard formatting, so that line counts reveal more information about where code regions begin and end. In those cases, we can use `#[rustfmt::skip]` to prevent that code from being disturbed.

``@rustbot`` label +A-code-coverage
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
coverage: Format all coverage tests with `rustfmt`

As suggested by <rust-lang/rust#119984 (comment)>.

Test files in `tests/` are normally ignored by `x fmt`, but sometimes those files end up being run through `rustfmt` anyway, either by `rust-analyzer` or by hand.

When that happens, it's annoying to have to manually revert formatting changes that are unrelated to the actual changes being made. So it's helpful for the tests in the repository to already have standard formatting beforehand.

However, there are several coverage tests that deliberately use non-standard formatting, so that line counts reveal more information about where code regions begin and end. In those cases, we can use `#[rustfmt::skip]` to prevent that code from being disturbed.

``@rustbot`` label +A-code-coverage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. WG-async Working group: Async & await
Projects
None yet
Development

Successfully merging this pull request may close these issues.