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

Upgrade to syn 2.0 #442

Closed
wants to merge 1 commit into from
Closed

Upgrade to syn 2.0 #442

wants to merge 1 commit into from

Conversation

lnicola
Copy link
Contributor

@lnicola lnicola commented May 1, 2023

No description provided.

@netlify
Copy link

netlify bot commented May 1, 2023

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 5258374
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/653ce91874ecaa0008e9ad31

@lnicola
Copy link
Contributor Author

lnicola commented May 1, 2023

This compiles, but doesn't work because of a run-time breaking change in syn:

error: expected parentheses
 --> tests/macros.rs:3:5
  |
3 |     #[salsa::invoke(another_module::another_name)]
  |     ^

@lnicola
Copy link
Contributor Author

lnicola commented May 1, 2023

CC @fee1-dead maybe you ran into this before (I noticed you've filed PRs against various projects)?

Comment on lines 689 to 690
let name = attr.path.segments[1].ident.to_string();
let tts = attr.tokens.into();
let name = attr.path().segments[1].ident.to_string();
let tts = attr.into_token_stream().into();
Copy link

@fee1-dead fee1-dead May 1, 2023

Choose a reason for hiding this comment

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

Doesn't this return the entire attribute? Previously tokens were the tokens that are not the path e.g. (foo) in #[salsa::something(foo)].

Copy link
Contributor Author

@lnicola lnicola May 1, 2023

Choose a reason for hiding this comment

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

Yeah, that's probably it. I looked at the MetaList in attr.meta, but couldn't really make it work, probably because that still includes the path.

Choose a reason for hiding this comment

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

The MetaList works differently. It doesn't have the path, but it doesn't have the parenthesis either. So you would have foo. You could probably quote! this and wrap the result around parentheses as a dirty hack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually working now AFAICT, it fails to build because of some Span/DelimSpan changes.

@Veykril
Copy link
Contributor

Veykril commented May 2, 2023

Would be nice if we could land #443 here as well should we cut another pre-release for old salsa (I don't think syn 2 demands changes for that PR)

@fee1-dead
Copy link

I think you could just use the DelimSpan's two spans in an array [span.open(), span.close()] and that should work

@lnicola
Copy link
Contributor Author

lnicola commented Jun 30, 2023

Thanks, that leaves this bit here, where Paren::span is a DelimSpan, but Ident::span() returns a Span and the only way to create a DelimSpan is from a Group, which you make from a Delimiter and a TokenStream.

@fee1-dead
Copy link

Try using the Group function.

@fee1-dead
Copy link

actually wait nvm. you wanted to create a paren token

@fee1-dead
Copy link

Ah, I think you can should still be able to call token::Paren(span) with a single span because of the impl here.

@lnicola
Copy link
Contributor Author

lnicola commented Jun 30, 2023

Thanks, this was such a struggle 😔.

@lnicola lnicola marked this pull request as ready for review June 30, 2023 13:03
@nikomatsakis
Copy link
Member

Hi @lnicola -- I apologize for letting this sit for so long. If you are still interested, would you consider rebasing it? If not, I understand.

@fee1-dead
Copy link

I cloned the repository, wanting to rebase this PR, but saw that the macros already use syn 2.0. Therefore there's no need to rebase this now.

@lnicola
Copy link
Contributor Author

lnicola commented Jun 19, 2024

Yup, I guess this is no longer necessary?

@lnicola lnicola closed this Jun 19, 2024
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.

4 participants