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

Is there a gentler way to land the assert_matches macro? #82913

Closed
anp opened this issue Mar 8, 2021 · 19 comments · Fixed by #86947
Closed

Is there a gentler way to land the assert_matches macro? #82913

anp opened this issue Mar 8, 2021 · 19 comments · Fixed by #86947
Assignees
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-resolve Area: Name resolution regression-from-stable-to-beta Performance or correctness regression from stable to beta. 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. T-release Relevant to the release subteam, which will review and decide on the PR/issue.
Milestone

Comments

@anp
Copy link
Member

anp commented Mar 8, 2021

As a user I'm really excited for std::assert_matches!, but I'm a little worried about the level of breakage it introduces and I'm hoping to discuss possible paths forward.

The Fuchsia roll to update rustc is currently blocked by ambiguous macro resolution errors with code that glob-imports matches::assert_matches!. It seems that a locally defined glob import does not take precedence over macros defined in std.

This implies a workaround of changing all the glob imports to use matches::assert_matches, but this does not account for submodules which use super::*;. Unfortunately it seems that the test_case crate wraps the function body in a submodule and adds a use super::*to the new module.

What this means in practice is that every function using the test_case crate must have an explicit import added or have the macro referenced via a fully qualified name. AIUI this change counts as an acceptable breakage and so I'm still putting together a diff which will resolve all of our roll blockers.

What I'm wondering though is whether knowing about this source of breakage offers a path to a smoother release. If so, we could revert the change and prepare to roll it out more gently.

Two options that came up in private discussions:

  1. remove assert_matches from the prelude until the 2021 edition
  • this wouldn't make it immediately available but it would prevent the breakage
  1. make local glob imports take precedence over std/prelude macros
  • this would make it immediately available but may not be possible within language constraints

Any other suggestions for how to make this change less disruptive? cc @m-ou-se @tmandry @joshtriplett

@estebank
Copy link
Contributor

estebank commented Mar 8, 2021

cc @petrochenkov for resolution

@estebank estebank added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-resolve Area: Name resolution T-libs-api Relevant to the library API 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 Mar 8, 2021
@anp
Copy link
Member Author

anp commented Mar 8, 2021

I've prepared a change which should allow us to update to the current nightly and might give a sense of the frequency within our codebase (more than I'd expect but less than I'd feared). I'm not sure to what extent that holds for the rest of the ecosystem, maybe a crater run would be useful to determine the severity here.

@tmandry tmandry added the T-release Relevant to the release subteam, which will review and decide on the PR/issue. label Mar 15, 2021
@tmandry
Copy link
Member

tmandry commented Mar 15, 2021

This was a big enough breakage for Fuchsia (and the error messages are confusing enough) that I think a crater run is warranted.

I'm not sure if the release team wants to weigh in at this point or not, but nominating for discussion in the release team meeting.

@rustbot label: +T-release +I-nominated

@joshtriplett
Copy link
Member

We discussed this in today's @rust-lang/libs meeting.

We'd propose the following:

  • Remove assert_matches! from the prelude, and only add it in a submodule. (Hopefully we don't have to extensively bikeshed the path to this.) People who want to use it can import it, for now.
  • Consider adding assert_matches! to the prelude in the next edition.

@Mark-Simulacrum
Copy link
Member

(I think the libs team decision makes this a bit moot, but perhaps not)

I think for unexpected, extensive breakage it usually makes sense to revert and perhaps reland with no changes, just giving folks a chance to evaluate the breakage - potentially via crater. I think the compiler team has been moving towards reverting eagerly, too, though there's not been set policy yet, and I think there's not quite consensus.

I'll leave this nominated in case others on the release team have thoughts and bring it up in our meeting too.

@Mark-Simulacrum Mark-Simulacrum added this to the 1.52.0 milestone Mar 19, 2021
@Mark-Simulacrum Mark-Simulacrum added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Apr 12, 2021
@Mark-Simulacrum
Copy link
Member

It looks like the removal here hasn't happened, and has now hit beta. Tagging as a beta regression.

@Mark-Simulacrum
Copy link
Member

#84759 takes care of the regression in beta (1.52), but we'll need a similar revert for 1.53 (branching ~today), and a proper fix for 1.54.

@pietroalbini pietroalbini added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels May 4, 2021
@pietroalbini pietroalbini modified the milestones: 1.52.0, 1.54.0 May 4, 2021
@pietroalbini
Copy link
Member

assert_matches has been reverted on the upcoming 1.53.0 beta too. Moving this to a nightly 1.54.0 regression.

@m-ou-se
Copy link
Member

m-ou-se commented Jun 4, 2021

* Remove `assert_matches!` from the prelude, and only add it in a submodule. (Hopefully we don't have to extensively bikeshed the path to this.) People who want to use it can import it, for now.

* Consider adding `assert_matches!` to the prelude in the next edition.

I'm not sure if this is a good way forward. We could definitely move it into a submodule, but making this an edition change (including a migration lint etc.) seems like too much effort for the small breakage this caused.

Having assert!() and assert_eq!() etc. always available, but assert_matches!() only after importing it with use std::bikeshed::assert_matches doesn't seem great.

As Mark suggested above, we could do the same as we did for usize::BITS, and re-land it now that hopefully most of the breakage is fixed in the ecosystem.

ping @rust-lang/libs for opinions

@BurntSushi
Copy link
Member

Landing this despite the breakage feels not so great to me. Not horrible. But not great. In particular, it looks like the root cause to the breakage is import behavior. Which I think also in turn means that adding any new macro to the prelude could potentially result in this sort of breakage, which seems really quite unfortunate. (Did this happen when the matches macro was added?) Does the @rust-lang/lang team have any opinions here? Is it feasible to fix the import behavior? Otherwise, the general migration path for a macro living in the ecosystem to a macro living in the prelude seems quite fraught.

With that said, can we do another crater run to see if the breakage has actually been fixed? If it has, then that might indeed be the right way to go here. But I'm still worried about the longer term problem with imports.

I'm not sure if this is a good way forward. We could definitely move it into a submodule, but making this an edition change (including a migration lint etc.) seems like too much effort for the small breakage this caused.

Having assert!() and assert_eq!() etc. always available, but assert_matches!() only after importing it with use std::bikeshed::assert_matches doesn't seem great.

Yeah I hear this. Having to import assert_matches kinda stinks. I don't have a good idea of how much work doing this via an edition is. We do have the upside that Rust 2021 is around the corner, so an explicit import wouldn't be needed for too long.

@SimonSapin
Copy link
Contributor

In particular, it looks like the root cause to the breakage is import behavior.

Indeed. Maybe that behavior could be fixed? Quoting myself from SimonSapin/rust-std-candidates#22 (comment):

I’ve managed to reproduce this.

error[E0659]: `assert_matches` is ambiguous (glob import vs any other name from outer scope during import/macro resolution)

It’s unfortunate that the compiler does not resolve this ambiguity automatically (favoring the explicit use over the prelude).

@m-ou-se
Copy link
Member

m-ou-se commented Jun 4, 2021

(Did this happen when the matches macro was added?)

Yes: #66518

(That problem was 'resolved' by ignoring it until it was too late because it reached the stable compiler: #66518 (comment))

Is it feasible to fix the import behavior? Otherwise, the general migration path for a macro living in the ecosystem to a macro living in the prelude seems quite fraught.

Apparetly that's tricky: #65721 (comment)

I don't have a good idea of how much work doing this via an edition is. We do have the upside that Rust 2021 is around the corner, so an explicit import wouldn't be needed for too long.

It'd be pretty much trivial to add it only in prelude::rust_2021. But we made a promise to the world to detect and automatically fix future edition-incompatible code, and that part is a bit harder. And we technically already froze the set of changes for new edition, although I guess nobody would complain too loudly if we still sneak this in. The problem would still remain for any macros we add in the future though.

@nikomatsakis
Copy link
Contributor

Having assert!() and assert_eq!() etc. always available, but assert_matches!() only after importing it with use std::bikeshed::assert_matches doesn't seem great.

@m-ou-se I'm confused by this -- if we did it in an edition, wouldn't all 3 be available? What makes this so hard to do in an edition? (I haven't thought about what migration is required)

@m-ou-se
Copy link
Member

m-ou-se commented Jun 4, 2021

Oh, the comment about std::bikeshed::assert_matches applies to Rust 2015 and 2018. (And Rust 2021 with #[no_prelude] i guess, since the others are imported through #[macro_use] and not the prelude.) What makes me reluctant to do it in the edition is specifically the migration. We'd need to detect cases where a macro with the same name is used that was imported through a glob import. It'd be a bit late to start working on a new migration lint now, since we're already supposed to be done with those by now.

And doing this edition trick won't work the next time when we run into this problem (without waiting three years). So I'd prefer some conslusion that we can re-use when this happens again in a few months or so.

@m-ou-se
Copy link
Member

m-ou-se commented Jun 14, 2021

Update: We don't have a solution here yet, and nightly is now at 1.55. So if we want to avoid the breakage, we need a backport again (to 1.54).

@m-ou-se m-ou-se added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Jun 23, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Jun 23, 2021

We discussed this today, and concluded that our best options here are either:

  1. Just let it land on stable, and break a few things.
  2. Only add it to the 2021 prelude, but without migration lint. We do normally require migration lints for edition changes, but this option is strictly less breaking than option 1.

To be discussed in the edition meeting on monday.

@m-ou-se
Copy link
Member

m-ou-se commented Jun 28, 2021

We discussed this in the edition meeting, and weren't too enthusiastic about making this an edition change without also adding an automatic migration for it. Breakage on upgrading to Rust 2021 without automatic migration is a reason for not upgrading at all, which we want to avoid. Adding a migration lint for this is also not easy.

@nikomatsakis encouraged a discussion of this general issue (macros from the std root/prelude breaking things) in a language meeting to see if something can be done about this problem. (As was suggested by @BurntSushi above.)

For now, we should move this macro to e.g. std::assert::assert_matches or something and backport that change to beta.

@m-ou-se m-ou-se self-assigned this Jul 7, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jul 15, 2021
…, r=yaahc

Move assert_matches to an inner module

Fixes rust-lang#82913
@bors bors closed this as completed in a5acb7b Jul 15, 2021
@Mark-Simulacrum
Copy link
Member

Going to reopen to track the beta backport.

@pietroalbini
Copy link
Member

Beta backport landed. Closing this.

bors added a commit to rust-lang-ci/rust that referenced this issue Feb 15, 2022
…ro, r=m-ou-se

Add a stack-`pin!`-ning macro to `core::pin`.

  - rust-lang#93178

`pin!` allows pinning a value to the stack. Thanks to being implemented in the stdlib, which gives access to `macro` macros, and to the private `.pointer` field of the `Pin` wrapper, [it was recently discovered](https://rust-lang.zulipchat.com/#narrow/stream/187312-wg-async-foundations/topic/pin!.20.E2.80.94.20the.20.22definitive.22.20edition.20.28a.20rhs-compatible.20pin-nin.2E.2E.2E/near/268731241) ([archive link](https://zulip-archive.rust-lang.org/stream/187312-wg-async-foundations/topic/A.20rhs-compatible.20pin-ning.20macro.html#268731241)), contrary to popular belief, that it is actually possible to implement and feature such a macro:

```rust
let foo: Pin<&mut PhantomPinned> = pin!(PhantomPinned);
stuff(foo);
```
or, directly:

```rust
stuff(pin!(PhantomPinned));
```

  - For context, historically, this used to require one of the two following syntaxes:

      - ```rust
        let foo = PhantomPinned;
        pin!(foo);
        stuff(foo);
        ```

      -  ```rust
         pin! {
             let foo = PhantomPinned;
         }
         stuff(foo);
         ```

This macro thus allows, for instance, doing things like:

```diff
fn block_on<T>(fut: impl Future<Output = T>) -> T {
    // Pin the future so it can be polled.
-   let mut fut = Box::pin(fut);
+   let mut fut = pin!(fut);

    // Create a new context to be passed to the future.
    let t = thread::current();
    let waker = Arc::new(ThreadWaker(t)).into();
    let mut cx = Context::from_waker(&waker);

    // Run the future to completion.
    loop {
        match fut.as_mut().poll(&mut cx) {
            Poll::Ready(res) => return res,
            Poll::Pending => thread::park(),
        }
    }
}
```

  - _c.f._, https://doc.rust-lang.org/1.58.1/alloc/task/trait.Wake.html

And so on, and so forth.

I don't think such an API can get better than that, barring full featured language support (`&pin` references or something), so I see no reason not to start experimenting with featuring this in the stdlib already 🙂

  - cc `@rust-lang/wg-async-foundations` \[EDIT: this doesn't seem to have pinged anybody 😩, thanks `@yoshuawuyts` for the real ping\]

r? `@joshtriplett`

___

# Docs preview

https://user-images.githubusercontent.com/9920355/150605731-1f45c2eb-c9b0-4ce3-b17f-2784fb75786e.mp4

___

# Implementation

The implementation ends up being dead simple (so much it's embarrassing):

```rust
pub macro pin($value:expr $(,)?) {
    Pin { pointer: &mut { $value } }
}
```

_and voilà_!

  - The key for it working lies in [the rules governing the scope of anonymous temporaries](https://doc.rust-lang.org/1.58.1/reference/destructors.html#temporary-lifetime-extension).

<details><summary>Comments and context</summary>

This is `Pin::new_unchecked(&mut { $value })`, so, for starters, let's
review such a hypothetical macro (that any user-code could define):
```rust
macro_rules! pin {( $value:expr ) => (
    match &mut { $value } { at_value => unsafe { // Do not wrap `$value` in an `unsafe` block.
        $crate::pin::Pin::<&mut _>::new_unchecked(at_value)
    }}
)}
```

Safety:
  - `type P = &mut _`. There are thus no pathological `Deref{,Mut}` impls that would break `Pin`'s invariants.
  - `{ $value }` is braced, making it a _block expression_, thus **moving** the given `$value`, and making it _become an **anonymous** temporary_.
    By virtue of being anonynomous, it can no longer be accessed, thus preventing any attemps to `mem::replace` it or `mem::forget` it, _etc._

This gives us a `pin!` definition that is sound, and which works, but only in certain scenarios:

  - If the `pin!(value)` expression is _directly_ fed to a function call:
    `let poll = pin!(fut).poll(cx);`

  - If the `pin!(value)` expression is part of a scrutinee:

    ```rust
    match pin!(fut) { pinned_fut => {
        pinned_fut.as_mut().poll(...);
        pinned_fut.as_mut().poll(...);
    }} // <- `fut` is dropped here.
    ```

Alas, it doesn't work for the more straight-forward use-case: `let` bindings.

```rust
let pinned_fut = pin!(fut); // <- temporary value is freed at the end of this statement
pinned_fut.poll(...) // error[E0716]: temporary value dropped while borrowed
                     // note: consider using a `let` binding to create a longer lived value
```

  - Issues such as this one are the ones motivating rust-lang/rfcs#66

This makes such a macro incredibly unergonomic in practice, and the reason most macros out there had to take the path of being a statement/binding macro (_e.g._, `pin!(future);`) instead of featuring the more intuitive ergonomics of an expression macro.

Luckily, there is a way to avoid the problem. Indeed, the problem stems from the fact that a temporary is dropped at the end of its enclosing statement when it is part of the parameters given to function call, which has precisely been the case with our `Pin::new_unchecked()`!

For instance,

```rust
let p = Pin::new_unchecked(&mut <temporary>);
```

becomes:

```rust
let p = { let mut anon = <temporary>; &mut anon };
```

However, when using a literal braced struct to construct the value, references to temporaries can then be taken. This makes Rust change the lifespan of such temporaries so that they are, instead, dropped _at the end of the enscoping block_.

For instance,
```rust
let p = Pin { pointer: &mut <temporary> };
```

becomes:

```rust
let mut anon = <temporary>;
let p = Pin { pointer: &mut anon };
```

which is *exactly* what we want.

Finally, we don't hit problems _w.r.t._ the privacy of the `pointer` field, or the unqualified `Pin` name, thanks to `decl_macro`s being _fully_ hygienic (`def_site` hygiene).

</details>

___

# TODO

  - [x] Add compile-fail tests with attempts to break the `Pin` invariants thanks to the macro (_e.g._, try to access the private `.pointer` field, or see what happens if such a pin is used outside its enscoping scope (borrow error));
  - [ ] Follow-up stuff:
      - [ ] Try to experiment with adding `pin!` to the prelude: this may require to be handled with some extra care, as it may lead to issues reminiscent of those of `assert_matches!`: rust-lang#82913
      - [x] Create the tracking issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-resolve Area: Name resolution regression-from-stable-to-beta Performance or correctness regression from stable to beta. 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. T-release Relevant to the release subteam, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.