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

use locations with call-site hygiene for UI errors #4666

Closed
wants to merge 2 commits into from

Conversation

davidhewitt
Copy link
Member

Fixes #4663

I happened to see the other day on Zulip that it's preferable to keep generate code using "call-site" hygiene rather than use user-provided spans, and to use Span::located_at to indicate where the user-provided input was problematic.

I made a quote_at_location! macro to replace our uses of quote_spanned!. This seems to work well, and adds the nice touch that it shows the proc-macro which caused the generated code (see the UI test changes).

It looks like this has (mostly) fixed #4663 too, however there's still a small test failure which I'll investigate later.

@Icxolu
Copy link
Contributor

Icxolu commented Oct 29, 2024

Hmm, interesting. Can you expand on why it is preferable to do so? From a quick search I could not really find usage of this in other popular macros. serde-rs/serde#2847 (comment) actually recommends against doing it.

and adds the nice touch that it shows the proc-macro which caused the generated code (see the UI test changes).

Is that really that useful? I feel like this has the tendency to clutter the error output and could possibly lead to the conclusion that the error is caused by the macro (our code) instead of incorrect user input code.

@davidhewitt
Copy link
Member Author

Hmm, you're right, this isn't as clear-cut as the the original zulip thread where I saw the suggestion that we should be doing this (https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/proc-macro.20span.20syntax.20context.20docs.20and.20best.20practices/near/478035702). Rust issue from that Zulip thread is at rust-lang/rust#130995

As far as I can tell from those threads, the idea is that using call_site hygiene will avoid the clippy and rustc lints from our generated code polluting user compiles. This seems desirable. I also thought it might simplify our macros if we're using call_site hygiene for more of the generated stuff (we've definitely had challenges around #py ident in particular occasionally being expanded in a mismatching context and not resolving).

As you have found like there is some discussion about what the right solution should be.

Is that really that useful? I feel like this has the tendency to clutter the error output and could possibly lead to the conclusion that the error is caused by the macro (our code) instead of incorrect user input code.

Good point. I agree it is cluttered too, though I saw it slightly differently in that that it might hint to users why their seemingly-ok code is problematic (i.e. because they haven't picked types suitable for Python), but yes it might just make them think PyO3 isn't working correctly.

Let's back out of this for now and wait until upstream has given a more consistent opinion on what they want. We can probably solve #4663 with a more targeted fix for now.

@Icxolu
Copy link
Contributor

Icxolu commented Oct 30, 2024

As far as I can tell from those threads, the idea is that using call_site hygiene will avoid the clippy and rustc lints from our generated code polluting user compiles. This seems desirable. I also thought it might simplify our macros if we're using call_site hygiene for more of the generated stuff (we've definitely had challenges around #py ident in particular occasionally being expanded in a mismatching context and not resolving).

I definitely agree here. In general we should keep span usage as minimal as needed and everything else can use call_site.

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.

#[pymethods] generated code triggers unsafe_op_in_unsafe_fn lint
2 participants