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

impl Trait not allowed after updating tracing-attributes:0.1.12 #1227

Closed
FintanH opened this issue Feb 5, 2021 · 3 comments · Fixed by #1233
Closed

impl Trait not allowed after updating tracing-attributes:0.1.12 #1227

FintanH opened this issue Feb 5, 2021 · 3 comments · Fixed by #1233

Comments

@FintanH
Copy link
Contributor

FintanH commented Feb 5, 2021

Bug Report

Hey 👋 thanks for taking the time to look at the following issue 😊

Version

The versions are:
tracing = 0.1.23
tracing-attributes = 0.1.12
tracing-core = 0.1.17

Here's the Cargo.lock of the reproducible.

Platform

Linux haptop 5.10.0-rc6 #1-NixOS SMP Sun Nov 29 23:50:50 UTC 2020 x86_64 GNU/Linux

Description

We're using a combination of tracing::instrument while also returning a trait object, i.e. impl. When this combined with Result and using err in tracing::instrument then we get the following compiler error:

error[E0562]: `impl Trait` not allowed outside of function and inherent method return types
  --> src/main.rs:22:30
   |
22 |     fn test(&self) -> Result<impl Iterator<Item = u32>, Error> {
   |                              ^^^^^^^^^^^^^^^^^^^^^^^^^

This was working before and it seems that an update of tracing-attributes caused the code not to compile. It's probably worth noting that this isn't an issue without the Result and err pragma.

Here's a minimal example of a project not compiling https://github.com/radicle-dev/tracing-bug. If you switch to the works branch, https://github.com/radicle-dev/tracing-bug/tree/works, then you can see that using tracing-attributes = "=0.1.11" allows the code to compile again.

If there's any more detail that would be helpful, please let me know ✌️ Thanks again!

@hawkw
Copy link
Member

hawkw commented Feb 5, 2021

Thanks for the report! I think the issue is due to PR #1006, which changed #[instrument(err)] to handle early error returns by wrapping the function body in a closure that's immediately invoked.

You'll notice here that we annotate that closure with the function's return type:

match move || #return_type #block () {
In the case where that return type contains an impl Trait, we can't use that type in a type annotation in this context, so this is invalid.

I think we should just be able to remove the type annotation and the compiler should still be able to infer the function's return type. If that isn't the case, and this is necessary to infer the type, I think we have to change the implementation to generate an inner function with the same prototype as the outer function, and call that, rather than using a closure. That might be somewhat more complicated, so hopefully removing the type annotation is sufficient.

Thanks again for the report — the repo case should be really helpful so we can easily write a test for this.

FintanH added a commit to radicle-dev/radicle-link that referenced this issue Feb 7, 2021
0.1.12 breaks our builds at the moment so we should specify an upper
bound of 0.1.11.

See: tokio-rs/tracing#1227

Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
xla pushed a commit to radicle-dev/radicle-link that referenced this issue Feb 9, 2021
0.1.12 breaks our builds at the moment so we should specify an upper
bound of 0.1.11.

See: tokio-rs/tracing#1227

Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
hawkw added a commit that referenced this issue Feb 9, 2021
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue Feb 9, 2021
Currently, using `#[instrument(err)]` on a function returning a `Result`
with an `impl Trait` in it results in a compiler error. This is because
we generate a type annotation on the closure in the function body that
contains the user function's actual body, and `impl Trait` isn't allowed
on types in local bindings, only on function parameters and return
types.

This branch fixes the issue by simply removing the return type
annotation from the closure. I've also added tests that break on master
for functions returning `impl Trait`, both with and without the `err`
argument.

Fixes #1227

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue Feb 10, 2021
…1233)

## Motivation

Currently, using `#[instrument(err)]` on a function returning a `Result`
with an `impl Trait` in it results in a compiler error. This is because
we generate a type annotation on the closure in the function body that
contains the user function's actual body, and `impl Trait` isn't allowed
on types in local bindings, only on function parameters and return
types.

## Solution

This branch fixes the issue by simply removing the return type
annotation from the closure. I've also added tests that break on master
for functions returning `impl Trait`, both with and without the `err`
argument.

Fixes #1227

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue Feb 11, 2021
…1233)

## Motivation

Currently, using `#[instrument(err)]` on a function returning a `Result`
with an `impl Trait` in it results in a compiler error. This is because
we generate a type annotation on the closure in the function body that
contains the user function's actual body, and `impl Trait` isn't allowed
on types in local bindings, only on function parameters and return
types.

## Solution

This branch fixes the issue by simply removing the return type
annotation from the closure. I've also added tests that break on master
for functions returning `impl Trait`, both with and without the `err`
argument.

Fixes #1227

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue Feb 11, 2021
…1236)

This backports PR #1233 to v0.1.x. This isn't *just* a simple cherry-pick
because the new tests in that branch use the v0.2.x module names, so
that had to be fixed. Otherwise, though, it's the same change, and I'll go
ahead and merge it when CI passes, since it was approved on `master`.

## Motivation

Currently, using `#[instrument(err)]` on a function returning a `Result`
with an `impl Trait` in it results in a compiler error. This is because
we generate a type annotation on the closure in the function body that
contains the user function's actual body, and `impl Trait` isn't allowed
on types in local bindings, only on function parameters and return
types.

## Solution

This branch fixes the issue by simply removing the return type
annotation from the closure. I've also added tests that break on master
for functions returning `impl Trait`, both with and without the `err`
argument.

Fixes #1227

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
Copy link
Member

hawkw commented Feb 17, 2021

@FintanH just wanted to let you know I've published tracing v0.1.24 with a fix for this! Thanks again for the helpful reproduction!

@FintanH
Copy link
Contributor Author

FintanH commented Feb 18, 2021

Amazing, thanks for keeping me in the loop and the quick turnaround @hawkw 😊 And also thank you for the great library ✌️

hawkw added a commit that referenced this issue Apr 30, 2021
# 0.1.26 (April 30, 2021)

### Fixed

- **attributes**: Compatibility between `#[instrument]` and `async-trait`
  v0.1.43 and newer ([#1228])
- Several documentation fixes ([#1305], [#1344])
### Added

- `Subscriber` impl for `Box<dyn Subscriber + Send + Sync + 'static>`
  ([#1358])
- `Subscriber` impl for `Arc<dyn Subscriber + Send + Sync + 'static>`
  ([#1374])
- Symmetric `From` impls for existing `Into` impls on `span::Current`,
  `Span`, and `Option<Id>` ([#1335], [#1338])
- `From<EnteredSpan>` implementation for `Option<Id>`, allowing
  `EnteredSpan` to be used in a `span!` macro's `parent:` field ([#1325])
- `Attributes::fields` accessor that returns the set of fields defined
  on a span's `Attributes` ([#1331])

Thanks to @Folyd, @nightmared, and new contributors @rmsc and @Fishrock123 for
contributing to this release!

[#1227]: #1228
[#1305]: #1305
[#1325]: #1325
[#1338]: #1338
[#1344]: #1344
[#1358]: #1358
[#1374]: #1374
[#1335]: #1335
[#1331]: #1331
hawkw added a commit that referenced this issue Apr 30, 2021
# 0.1.26 (April 30, 2021)

### Fixed

- **attributes**: Compatibility between `#[instrument]` and `async-trait`
  v0.1.43 and newer ([#1228])
- Several documentation fixes ([#1305], [#1344])
### Added

- `Subscriber` impl for `Box<dyn Subscriber + Send + Sync + 'static>`
  ([#1358])
- `Subscriber` impl for `Arc<dyn Subscriber + Send + Sync + 'static>`
  ([#1374])
- Symmetric `From` impls for existing `Into` impls on `span::Current`,
  `Span`, and `Option<Id>` ([#1335], [#1338])
- `From<EnteredSpan>` implementation for `Option<Id>`, allowing
  `EnteredSpan` to be used in a `span!` macro's `parent:` field ([#1325])
- `Attributes::fields` accessor that returns the set of fields defined
  on a span's `Attributes` ([#1331])

Thanks to @Folyd, @nightmared, and new contributors @rmsc and @Fishrock123 for
contributing to this release!

[#1227]: #1228
[#1305]: #1305
[#1325]: #1325
[#1338]: #1338
[#1344]: #1344
[#1358]: #1358
[#1374]: #1374
[#1335]: #1335
[#1331]: #1331
kaffarell pushed a commit to kaffarell/tracing that referenced this issue May 22, 2024
# 0.1.26 (April 30, 2021)

### Fixed

- **attributes**: Compatibility between `#[instrument]` and `async-trait`
  v0.1.43 and newer ([tokio-rs#1228])
- Several documentation fixes ([tokio-rs#1305], [tokio-rs#1344])
### Added

- `Subscriber` impl for `Box<dyn Subscriber + Send + Sync + 'static>`
  ([tokio-rs#1358])
- `Subscriber` impl for `Arc<dyn Subscriber + Send + Sync + 'static>`
  ([tokio-rs#1374])
- Symmetric `From` impls for existing `Into` impls on `span::Current`,
  `Span`, and `Option<Id>` ([tokio-rs#1335], [tokio-rs#1338])
- `From<EnteredSpan>` implementation for `Option<Id>`, allowing
  `EnteredSpan` to be used in a `span!` macro's `parent:` field ([tokio-rs#1325])
- `Attributes::fields` accessor that returns the set of fields defined
  on a span's `Attributes` ([tokio-rs#1331])

Thanks to @Folyd, @nightmared, and new contributors @rmsc and @Fishrock123 for
contributing to this release!

[tokio-rs#1227]: tokio-rs#1228
[tokio-rs#1305]: tokio-rs#1305
[tokio-rs#1325]: tokio-rs#1325
[tokio-rs#1338]: tokio-rs#1338
[tokio-rs#1344]: tokio-rs#1344
[tokio-rs#1358]: tokio-rs#1358
[tokio-rs#1374]: tokio-rs#1374
[tokio-rs#1335]: tokio-rs#1335
[tokio-rs#1331]: tokio-rs#1331
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants