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

#[instrument] does not warn on unrecognized options #727

Closed
jonhoo opened this issue May 21, 2020 · 2 comments · Fixed by #786
Closed

#[instrument] does not warn on unrecognized options #727

jonhoo opened this issue May 21, 2020 · 2 comments · Fixed by #786
Labels
crate/attributes Related to the `tracing-attributes` crate kind/feature New feature or request

Comments

@jonhoo
Copy link
Contributor

jonhoo commented May 21, 2020

Bug Report

Version

tracing 0.1.14

Description

The following code currently compiles without warnings or errors:

use tracing::instrument;

#[instrument(debug)]
fn foo() {}

A particularly unobservant user (like myself) may try this without reading the documentation carefully, and will likely be very confused why foo shows up even at info level. It'd be good to warn when unsupported attribute options are used, and ideally also suggest what to use instead (level = "debug") for things like debug/trace/etc. We probably don't want it to be a hard error for forward-compatibility.

Separately, wouldn't it be kind of nice to support this short-hand syntax? :p

@hawkw
Copy link
Member

hawkw commented May 21, 2020

#672 will make instrument error on unrecognized syntax. I'm not sure if we can easily emit warnings rather than errrors from proc macros, but I can take a look at it.

Separately, wouldn't it be kind of nice to support this short-hand syntax? :p

Sure, we could probably add something like that!

@hawkw hawkw added crate/attributes Related to the `tracing-attributes` crate kind/feature New feature or request labels May 21, 2020
@hawkw hawkw added this to the tracing-attributes 0.2 milestone Jul 7, 2020
@hawkw
Copy link
Member

hawkw commented Jul 7, 2020

Well, it turns out that there isn't currently a good way to emit custom warnings from proc macros on stable (there is a nightly-only API, though!).

However, I figured out a bit of a hack to turn the failure on unrecognized input from #627 into a warning:
Screenshot_20200707_112812
it's not the greatest, since it's not actually a deprecation, but at least it emits an actual warning, and works on stable. Unlike #672, this is not a breaking change.

@hawkw hawkw closed this as completed in #786 Jul 8, 2020
hawkw added a commit that referenced this issue Jul 8, 2020
## Motivation

In PR #672, the `#[instrument]` attribute was rewritten to use a custom
`syn` parser rather than using `syn::AttributeArgs`. This was primarily
to allow the use of arbitrary expressions as fields, but it had the side
benefit of also allowing more sophisticated error reporting. In
particular, we were able to replace the previous behavior of silently
ignoring unknown input with a compiler error, which has been requested a
few times (see #772 and #727). However, emitting a compiler error for
inputs that we previously allowed (even if they did nothing) is
technically a breaking change --- see
#773 (comment) and
following comments. This means that the other new features added in #672
cannot be released until we release `tracing-subscriber` 0.2.

## Solution 

This branch avoids the breaking change by downgrading these errors to
warnings. However, it isn't currently possible to emit a warning from a
proc-macro on stable --- the `proc_macro::Diagnostic` API which would
allow this requires a nightly feature. This branch hacks around this by
generating a fake deprecated item with for each unrecognized token in
the input, emitting the skipped item's parse error as the deprecation's
`note`, and "using" the deprecated item.

The deprecated warning is used here rather than other code that will
emit compiler warnings, because it gives us a way to customize the
warning output (via the `note` field). I've tried to make it fairly
obvious that this is not a "real" deprecation. Sample output looks like
this:
![Screenshot_20200707_153151](https://user-images.githubusercontent.com/2796466/86850948-fce0a780-c066-11ea-9b1c-5085c0fd43e0.png)

Fixes #727 
Fixes #772 

Signed-off-by: Eliza Weisman <eliza@elizas.website>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/attributes Related to the `tracing-attributes` crate kind/feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants