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

Remove usage of unmaintained crate proc-macro-error. #41

Merged

Conversation

AurelienFT
Copy link
Contributor

Hi,
Thanks for this crate :)

Resolves https://rustsec.org/advisories/RUSTSEC-2024-0370.html

If that's possible to do a realase aftter merging this that would be awesome :)

Copy link
Contributor

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

This has a security advisory now? Ah, simply "unmaintained".

This is something I've been aware of for a while, however proc-macro2-diagnostics is not a drop-in replacement (see SergioBenitez/proc-macro2-diagnostics#8). This would be easier if rust-lang/rust#54140 would finally get resolved!

Comment on lines 306 to 307
emit_error!(item, "expected struct");
item.span().error("expected struct").emit_as_expr_tokens();
Toks::new()
Copy link
Contributor

@dhardy dhardy Oct 21, 2024

Choose a reason for hiding this comment

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

This is not a direct replacement. IIUC the new code here does nothing on stable. The old code would actually emit an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh okey I was thinking that this was emitting looking at the other crates that made this migration. Sorry I'm not very familiar with details of proc-macro. Will try to drop-in replacement with the crate you mentioned.

Copy link
Contributor

Choose a reason for hiding this comment

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

I get the feeling that other people may have made this mistake.

I started working on this including error-verification a while back, but gave up: https://github.com/kas-gui/impl-tools/commits/work/
Might as well just do the switch to proc-macro-error2 now though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think a lot of proc-macro crate lack of error tests, I think mainly because they are boring/complex/.. to do, maybe with new IA tools it could be improved.

@dhardy
Copy link
Contributor

dhardy commented Oct 21, 2024

I see that https://crates.io/crates/proc-macro-error2 is now a thing. That would be the simplest replacement.

@dhardy
Copy link
Contributor

dhardy commented Oct 21, 2024

I don't see any breaking changes and this dependency is not part of the public API, so we should be able to bump the version to 0.10.1.

@AurelienFT AurelienFT force-pushed the remove_dependency_proc_macro_error branch from 13b1748 to 07ba7a9 Compare October 21, 2024 16:22
@AurelienFT AurelienFT requested a review from dhardy October 21, 2024 16:22
Copy link
Contributor

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Easy 😀

@dhardy
Copy link
Contributor

dhardy commented Oct 21, 2024

MSRV fails because the new dep requires 1.61.

Uh, I'll fix it in a new PR.

@dhardy dhardy merged commit 3d1cc86 into kas-gui:master Oct 21, 2024
4 of 5 checks passed
@AurelienFT
Copy link
Contributor Author

Oopsie didn't saw CI results.

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.

2 participants