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 attribute syntax instead of magic comments #56

Closed
sgrif opened this issue Aug 9, 2018 · 10 comments
Closed

Use attribute syntax instead of magic comments #56

sgrif opened this issue Aug 9, 2018 · 10 comments

Comments

@sgrif
Copy link

sgrif commented Aug 9, 2018

You'd asked me to open an issue to discuss strategy, so here we go. There are basically two options. Ultimately the whole thing is getting passed to a derive, so the simplest option is to not have impl_web! spit out the tokens its given unmodified, and instead have derive_resource! spit out its input (with the recognized attributes stripped) in addition to what it spits out today.

You mentioned you had a solution which works and messes up error messages, so I'm assuming it's the solution above. The second option would be to run the tokens passed to impl_web! through another macro before outputting them. This would be a basic macro_rules! macro, which strips out regonized meta items.

Both of these options are pretty simple to implement. Diesel does both in multiple places (depending on whether our macro is fundamentally a macro_rules! or procedural macro). Would love to hear more about whether this would somehow affect your error messages.

@carllerche
Copy link
Owner

Yeah, I tried the first route and that basically broke all error messages. I thought of the second strategy but I am not versed in regular macros and it seemed hard.

If this works and maintains good error messages I would be happy to switch to it.

@sgrif
Copy link
Author

sgrif commented Aug 9, 2018

Do you have any details on how it broke error messages, or a script you can share to ensure it retains similar output that I can use to determine whether errors have been broken?

@carllerche
Copy link
Owner

Right now, code errors in the impl_web block are maintained. So you get line numbers and all that.

When I mutated the actual code in the impl_web block all that was lost.

@sgrif
Copy link
Author

sgrif commented Aug 9, 2018

I don't suppose you have that in the form of a ui test? (e.g. with compiletest-rs)

@carllerche
Copy link
Owner

Unfortunately not

@sgrif
Copy link
Author

sgrif commented Aug 9, 2018

I'd love some guidance here, then. I'm still unclear on what exactly broke in your previous attempts, or how I can verify that I haven't caused the same problem

@cramertj
Copy link

@sgrif I believe (I could be wrong though) that modifying TokenStreams rather than pushing them straight through using the current stable proc_macro API will result in the whole thing being stringified, which breaks source location. The updated proc_macro interface fixes this, but it isn't stable yet and so the higher-quality error messages would only be available using nightly. There's some more information on this in the README of the proc-macro2 crate.

@sgrif
Copy link
Author

sgrif commented Aug 10, 2018

@cramertj I'm familiar with the current state of hygiene and span information. :) The relevant code is coming from pieces of a doc comment though, which should never be appearing in error messages either way.

@cramertj
Copy link

@sgrif I think the problem comes from having to strip the attributes away in the final result. Because custom attributes aren't stable, they have to be removed from the generated result, which breaks span information for the rest of the code inside the macro.

@dtolnay dtolnay mentioned this issue Aug 10, 2018
@dtolnay
Copy link
Contributor

dtolnay commented Aug 10, 2018

I submitted #59 with a fix that does not disrupt error reporting.

impl_web! {
    impl JsonResource {
        #[get("/")]
        fn hello_world(&self) -> Result<serde_json::Value, ()> {
            Ok(json!({ "message": "hello world" }))
        }
    }
}

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

No branches or pull requests

4 participants