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.

Sign up for GitHub

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

Jump to bottom

attributes: speculatively expand when parsing fails #1634

Merged
merged 1 commit into from Oct 17, 2021
Merged

attributes: speculatively expand when parsing fails #1634

merged 1 commit into from Oct 17, 2021

Conversation

cynecx
Copy link
Contributor

@cynecx cynecx commented Oct 12, 2021

Fixes #1633.

I intentially did not open a PR because the code isn't really polished and I wasn't sure if that is the right approach. I just needed something quickly to improve my rust-analyzer experience with a project. But here we go...

TODO: Check approach, polish/optimize code (eg. I wasn't sure if cloning a TokenStream is "zero/low-cost").

@cynecx cynecx requested review from davidbarsky, hawkw and a team as code owners October 12, 2021 17:24
@hawkw hawkw marked this pull request as draft October 12, 2021 17:29
@hawkw
Copy link
Member

hawkw commented Oct 12, 2021

Thanks for opening the PR!

I converted this to a draft while it's a work in progress. Feel free to mark it as "ready for review" once you think the implementation is ready.

@hawkw
Copy link
Member

hawkw commented Oct 12, 2021

I haven't had a chance to actually look closely at either PR yet, but tokio recently made a similar change to the #[tokio::main] macro: tokio-rs/tokio#4162

It might be worth comparing this approach to that branch?

hawkw
hawkw reviewed Oct 13, 2021
View reviewed changes
tracing-attributes/src/lib.rs Outdated Show resolved Hide resolved
tracing-attributes/src/lib.rs Outdated Show resolved Hide resolved
tracing-attributes/src/lib.rs Outdated Show resolved Hide resolved
tracing-attributes/src/lib.rs Outdated Show resolved Hide resolved
tracing-attributes/src/lib.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Oct 14, 2021

TODO: Check approach, polish/optimize code (eg. I wasn't sure if cloning a TokenStream is "zero/low-cost").

I'm also not sure about the performance implications of cloning a TokenStream...although I notice that the Tokio PR I linked in #1634 (comment) does it as well: https://github.com/tokio-rs/tokio/pull/4162/files#diff-3778ff38bebb2786ca2b45f4704af832e54cd75369b7b9215eae010cddc58a09R381-R387

Looking at the syn docs, though, I noticed the existence of ParseBuffer::fork, which seems to be intended for this kind of speculative parsing? I wonder if ParseBuffer::fork would provide better performance than TokenStream::clone...though I'm not sure if this is the case.

Changing this branch to use ParseBuffer::fork might be a bigger change, though, since we'd need to refactor this code so that it all occurs inside of a syn::Parse implementation (as this is the only way to get a ParseStream; the ParseStream/ParseBuffer type isn't publicly construct-able).

I don't immediately know whether or not this is worth doing, but I thought it could be good to mention?

@hawkw
Copy link
Member

hawkw commented Oct 15, 2021

ping @cynecx are you still interested in working on moving this forward?

i'd like to get the IDE support issue fixed as soon as possible, so if you're busy, I'd be happy to take over getting this PR ready to merge. of course, we'd credit your work when merging even if I wrapped up this change. if you're still planning to work on it, that's totally fine --- i just thought i'd offer in the interest of fixing the issue as quickly as possible.

thanks again for your work on this, in either case!

@cynecx
Copy link
Contributor Author

cynecx commented Oct 15, 2021 •
edited

@hawkw Sorry about the delay (I haven't had time to work on this until now)! Yes, I am still interested in moving this forward. I'll address the comments asap :)

@cynecx cynecx force-pushed the spec-expand branch 2 times, most recently from cd5c860 to fef5384 Compare October 15, 2021 20:16
cynecx
cynecx commented Oct 15, 2021
View reviewed changes
tracing-attributes/src/lib.rs Outdated Show resolved Hide resolved
@cynecx cynecx changed the title [WIP] attributes: speculative expand when parsing fails [WIP] attributes: speculatively expand when parsing fails Oct 15, 2021
@cynecx cynecx changed the title [WIP] attributes: speculatively expand when parsing fails attributes: speculatively expand when parsing fails Oct 15, 2021
@cynecx cynecx marked this pull request as ready for review October 15, 2021 20:51
@cynecx
Copy link
Contributor Author

cynecx commented Oct 15, 2021

I'm also not sure about the performance implications of cloning a TokenStream

If I understand correctly, cloning the TokenStream might be actually very cheap since it's reference counted internally (Assuming that the proc_macro bridge is actually rpcing calls to the TokenStream impl inside rustc_ast):

https://github.com/rust-lang/rust/blob/265fef45f20e3b8ed9495201b5d02bab1210f0f9/compiler/rustc_ast/src/tokenstream.rs#L322

@hawkw
Copy link
Member

hawkw commented Oct 15, 2021

If I understand correctly, cloning the TokenStream might be actually very cheap since it's reference counted internally (Assuming that the proc_macro bridge is actually rpcing calls to the TokenStream impl inside rustc_ast):

Ah, in that case, I think this approach is fine! It might be nice to have a comment noting that this is cheap, but I don't think it's really a blocker in any case.

hawkw
hawkw approved these changes Oct 16, 2021
View reviewed changes
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

looks good to me, modulo one question about a comment

tracing-attributes/src/lib.rs Outdated Show resolved Hide resolved
tracing-attributes/src/lib.rs Outdated Show resolved Hide resolved
@cynecx
Copy link
Contributor Author

cynecx commented Oct 16, 2021

@hawkw Thanks for the feedback. I’ll address the comments when I am back home… :)

@cynecx
Copy link
Contributor Author

cynecx commented Oct 17, 2021

@hawkw Btw, I've addressed your comments :)

hawkw
hawkw approved these changes Oct 17, 2021
View reviewed changes
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Okay, this looks good to me! Thanks @cynecx for all your hard work on this!

@hawkw hawkw merged commit 202632c into tokio-rs:master Oct 17, 2021
@lnicola lnicola mentioned this pull request Oct 17, 2021
Closed
@cynecx cynecx deleted the spec-expand branch October 17, 2021 17:58
@cynecx
Copy link
Contributor Author

cynecx commented Oct 17, 2021

@hawkw Also, Thank You for adding a proper commit message :D

hawkw pushed a commit that referenced this pull request Oct 22, 2021
@cynecx @hawkw
attributes: speculative expand when parsing fails (#1634)
f9fbfe3 
## Motivation

Recent `rust-analyzer` versions enabled automatic expansion of proc
macro attributes by default. This is a problem with `#[instrument]`,
because it currently produces a `compile_error!` when parsing the code
inside the `#[instrument]`ed function fails, and *discards* those
tokens. This means that if the `#[instrument]` attribute is placed on a
function whose implementation fails to parse, recent versions of
`rust-analyzer` will no longer be able to display diagnostics for those
errors. In some cases, this can also break autocompletion.

## Solution

This branch changes `#[instrument]` to always expand to the tokens
contained in the `#[instrument]`ed function body, regardless of whether
or not they could be parsed successfully. Now, an error is only emitted
when the `#[instrument]` attribute *itself* could not be parsed. Since
the instrumented function is always expanded, any errors within that
function can be displayed properly by `rust-analyzer`.

Fixes #1633.
@hawkw hawkw mentioned this pull request Oct 22, 2021
Merged
hawkw pushed a commit that referenced this pull request Oct 22, 2021
@cynecx @hawkw
attributes: speculative expand when parsing fails (#1634)
8540abc 
## Motivation

Recent `rust-analyzer` versions enabled automatic expansion of proc
macro attributes by default. This is a problem with `#[instrument]`,
because it currently produces a `compile_error!` when parsing the code
inside the `#[instrument]`ed function fails, and *discards* those
tokens. This means that if the `#[instrument]` attribute is placed on a
function whose implementation fails to parse, recent versions of
`rust-analyzer` will no longer be able to display diagnostics for those
errors. In some cases, this can also break autocompletion.

## Solution

This branch changes `#[instrument]` to always expand to the tokens
contained in the `#[instrument]`ed function body, regardless of whether
or not they could be parsed successfully. Now, an error is only emitted
when the `#[instrument]` attribute *itself* could not be parsed. Since
the instrumented function is always expanded, any errors within that
function can be displayed properly by `rust-analyzer`.

Fixes #1633.
@asdrubalini asdrubalini mentioned this pull request Oct 23, 2021
Closed
hawkw added a commit that referenced this pull request Feb 4, 2022
@hawkw
attributes: prepare to release 0.1.19
d65827b 
# 0.1.19 (February 3, 2022)

This release introduces a new `#[instrument(ret)]` argument to emit an
event with the return value of an instrumented function.

### Added

- `#[instrument(ret)]` to record the return value of a function
  ([#1716])
- added `err(Debug)` argument to cause `#[instrument(err)]` to record
  errors with `Debug` rather than `Display ([#1631])

### Fixed

- incorrect code generation for functions returning async blocks
  ([#1866])
- incorrect diagnostics when using `rust-analyzer` ([#1634])

Thanks to @Swatinem, @hkmatsumoto, @cynecx, and @ciuncan for
contributing to this release!

[#1716]: #1716
[#1631]: #1631
[#1634]: #1634
[#1866]: #1866
@hawkw hawkw mentioned this pull request Feb 4, 2022
Merged
hawkw added a commit that referenced this pull request Feb 4, 2022
@hawkw
attributes: prepare to release 0.1.19
46eda1e 
# 0.1.19 (February 3, 2022)

This release introduces a new `#[instrument(ret)]` argument to emit an
event with the return value of an instrumented function.

### Added

- `#[instrument(ret)]` to record the return value of a function
  ([#1716])
- added `err(Debug)` argument to cause `#[instrument(err)]` to record
  errors with `Debug` rather than `Display ([#1631])

### Fixed

- incorrect code generation for functions returning async blocks
  ([#1866])
- incorrect diagnostics when using `rust-analyzer` ([#1634])

Thanks to @Swatinem, @hkmatsumoto, @cynecx, and @ciuncan for
contributing to this release!

[#1716]: #1716
[#1631]: #1631
[#1634]: #1634
[#1866]: #1866
@davidbarsky davidbarsky mentioned this pull request Sep 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hawkw hawkw hawkw approved these changes

@davidbarsky davidbarsky Awaiting requested review from davidbarsky davidbarsky is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

#[instrument] attribute is not IDE friendly
2 participants
@cynecx @hawkw