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

attributes: prepare to release 0.1.9 #773

Merged
merged 2 commits into from
Jul 8, 2020
Merged

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Jun 30, 2020

0.1.9 (July 8, 2020)

Added

Changed

Fixes #785

@hawkw hawkw requested a review from a team as a code owner June 30, 2020 21:21
@hawkw
Copy link
Member Author

hawkw commented Jun 30, 2020

It occurs to me that the second change in the changelog, emitting errors for unrecognized input to instrument rather than ignoring it, maybe should be considered a semver breaking change, as it could cause code that previously compiled to no longer compile. However, adding an error for incorrect code that was previously accepted could be considered a "bugfix"...

@carllerche do you have any thoughts on how this interacts with tokio's versioning policies?

@hawkw
Copy link
Member Author

hawkw commented Jun 30, 2020

In an ideal world, we would emit warnings rather than errors, and this would definitely not be breaking...but unfortunately, there isn't currently a good way for proc macros to emit warnings.

@LucioFranco
Copy link
Member

I think its fine tbh, curious what @davidbarsky thinks too?

@LucioFranco
Copy link
Member

How many people do you think are using tracing in such a way that it would affect them rather than help them? (even if its breaking their code)

@hawkw
Copy link
Member Author

hawkw commented Jun 30, 2020

How many people do you think are using tracing in such a way that it would affect them rather than help them? (even if its breaking their code)

Hmm. I mean, it's helping everyone, because they may be unaware that their code is actually not doing anything. But, if the incorrect code exists in the wild, it might e.g. break CI builds that were previously passing.

@yaahc
Copy link
Collaborator

yaahc commented Jun 30, 2020

as it could cause code that previously compiled to no longer compile

this is a pretty critical reason to increment the semver imo. I'm thinking about cases where I've had semver bit me. Most recently on a big project I hadn't touched in a year where it stopped compiling because amethyst or one of its deps fked up semver. I couldn't easily fix the issue other than figuring out the correct exact version or upgrading to the next version of amethyst which was more than I was willing to do so I just gave up trying to play with it and wrote off the codebase as dead. I can see this same issue cropping up if someone has a dependency that stops compiling because of this change, where its not an easy change to their own code to fix.

On the other side, if we do a major semver bump could any of this cause compilation failures if two different versions of this crate are linked in? I recently did a small breaking change bugfix to eyre and almost immediately got a bug report because someone had a crate that exposed color-eyre as part of its interface and then a user bumped their version to the newest one and couldn't figure out why it wouldn't compile anymore.

It seems like the former concern is the only one likely to be an issue for tracing-attributes, so I lean towards calling this a breaking change.

@hawkw
Copy link
Member Author

hawkw commented Jun 30, 2020

On the other side, if we do a major semver bump could any of this cause compilation failures if two different versions of this crate are linked in?

This is especially concerning because tracing re-exports tracing-attributes. The hope was that breaking changes to the tracing-attributes crate could be decoupled while allowing users to opt in to the new version of tracing-attributes by depending on it explicitly. We should make sure that works before doing a semver bump --- otherwise, this will have to wait until we've prepared all the necessary breaking changes for tracing.

@hawkw
Copy link
Member Author

hawkw commented Jul 1, 2020

If this does end up requiring a semver bump change, we should make sure to also change the behavior with empty fields in instrument as I described in #672 (comment)

One thing that's worth noting is that the current implementation will
generate an empty field when a field name is provided without an =
and a value. This makes sense when only simple fields with literal
values are permitted. However, if we accept arbitrary expressions in the
macro, this is not the ideal behavior --- we would prefer to use the
same local variable shorthand as the function-like tracing macros.
However, changing this now is a breaking change. Any code which uses a
name that doesn't exist in the current scope to declare an empty field
would fail to compile, because it attempts to reference a name that
doesn't exist. Instead, I left a comment noting that this is not the
ideal behavior and it should be changed next time we're ready to break
the proc macros.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request 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>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
Copy link
Member Author

hawkw commented Jul 8, 2020

Okay, I've rebased to pick up #786 and updated the changelog. Now, we should emit warnings rather than errors for unrecognized input, meaning that this is no longer a potentially breaking change.

@hawkw hawkw requested a review from yaahc July 8, 2020 16:38
@hawkw hawkw merged commit 139d248 into master Jul 8, 2020
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 this pull request may close these issues.

instrument macro doesn't allow declaration of fields that have a period . in their name
4 participants