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

Swap to Proc Macro Error 2 RUSTSEC-2024-0370 #350

Merged
merged 15 commits into from
Oct 25, 2024
Merged

Swap to Proc Macro Error 2 RUSTSEC-2024-0370 #350

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
c8b5f54
swap to proc_macro_error2
pvichivanives Sep 6, 2024
489e0df
trybuild=Overwrite
pvichivanives Sep 9, 2024
58cfbab
some stderr diff in 1.79
pvichivanives Sep 12, 2024
ec3c48b
add nightly
pvichivanives Sep 17, 2024
26d28c0
derive nightly features
pvichivanives Sep 18, 2024
f32150b
add Features to Readme, Changelog stuff
pvichivanives Oct 1, 2024
27a37de
add feature flags instead of all
pvichivanives Oct 7, 2024
36970c2
bump msrv to 1.81
pvichivanives Oct 7, 2024
ba4e374
fix
pvichivanives Oct 12, 2024
e7668f2
use nightly toolchain?
pvichivanives Oct 12, 2024
342bf45
fix all features
pvichivanives Oct 14, 2024
c2402e9
remove extra space
pvichivanives Oct 14, 2024
77bc98e
Fix
pvichivanives Oct 14, 2024
030cf45
fix
pvichivanives Oct 14, 2024
60e7d51
disable with all-features
pvichivanives Oct 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion validator_derive/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ proc-macro = true
syn = "2"
quote = "1"
proc-macro2 = "1"
proc-macro-error = "1"
proc-macro-error2 = "2"
pvichivanives marked this conversation as resolved.
Show resolved Hide resolved
darling = { version = "0.20", features = ["suggestions"] }
once_cell = "1.18.0"

[features]
nightly = ["proc-macro-error2/nightly"]
Copy link
Owner

Choose a reason for hiding this comment

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

can the name be more clear somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nightly_features?

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure, I don't even know what this does in proc-macro-error2

Copy link
Contributor Author

@pvichivanives pvichivanives Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For the nightly feature in proc-macro-error2 its just a different implementation for emitting diagnostics and aborting. The main difference between the two appear to be that warnings are emitted in nightly while ignored in non-nightly builds. Tagging @HigherOrderLogic since they requested this.

I also think its fine if the feature isn't included since Validator might not want to use these extra warnings anyway. Might be a good idea to move this to a different PR for another discussion.

If you want to look at the code its here: https://github.com/GnomedDev/proc-macro-error-2/tree/master/src/imp
The delegate.rs is the nightly version and fallback.rs is the non nightly version

Copy link

Choose a reason for hiding this comment

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

I'm not sure, I don't even know what this does in proc-macro-error2

It's as @pvichivanives said, this feature enables emitting warning when using nightly compiler, otherwise ignored.

So to match the function, I think this feature can be renamed to nightly-warnning, or nightly-proc-macro, or even the longer nightly-proc-macro-error

Copy link

Choose a reason for hiding this comment

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

I've written this in #351, but I'd like to point it out here too: proc-macro-error2 fundamentally does not do the same thing as proc-macro-error, which is to avoid the need to have the user switch on a separate crate feature to get better proc macro diagnostics on nightly. The entire point of proc-macro-error is that both sets of error reporting functionality (via compiler_error! or via the nightly proc macro diagnostics API) are available to proc macro authors already, but instead of the library author having to decide (or add a feature), whether the nightly APIs can be used is detected at build time via build.rs. Using proc-macro-error2 still provides a shim around #[cfg(nightly_macro_diagnostics)] so there's a single API for the macro code no matter the compiler version, but doesn't do the auto-detection.

If this is fine for you @Keats, then this might not be a concern for this PR, but you should make sure that this is actually what you want. In particular, I've linked a different crate in the issue above (proc-macro2-diagnostics) that's a similar kind of drop-in replacement but which does do the build-time compiler detection. To me, this would definitely be preferable because IMO the point of using proc-macro-error is to automatically give good error messages where possible. At minimum, if you decide to go forwards with this PR and proc-macro-error2, the feature should receive some documentation in the crate docs. Otherwise, no one's gonna know what it does or turn it on, because most even nitghtly users will probably not know what a proc-macro-error2 is or what it does and that they could be getting much better error messages (which again is why I think it would be better if they didn't have to know at all).

Copy link
Owner

Choose a reason for hiding this comment

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

Honestly I don't really care which version gets used. The least disturbance the better though. If someone took proc-macro-error and just bumped syn without any other changes it would be fine. But I agree that needing a feature for some nightly stuff is not great

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to swap over the pr/close this pr if you would want to make one @domenicquirl. Am slightly concerned though that proc-macro2-diagnostics has 1 commit in the last 12 months, being a version bump which hasn't been released yet (last release was around 1year ago). Also SergioBenitez/version_check#23 <- for this as a rust team dev pointed out they would prefer people didn't automatically detect nightly.

Copy link

Choose a reason for hiding this comment

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

I wouldn't be too worried about that, Sergio has been active in the issues over there as recent as a month ago. It's just a small crate that doesn't need lots of updating.

Aside from that, point taken on nightly detection in general. It's maybe worth pointing out that there's also a PR to proc-macro2-diagnostics that tries to put their nightly detection (the detection itself, not the usage of the nightly APIs) behind a feature flag.

Personally, I don't use validator in a nightly context, which means I don't get nice diagnostics either way, so I probably won't push for a specific alternative solution as long as the syn version gets bumped with the switch. Just wanted to make sure that the change in behaviour is known, and as I said above I think you should add a section to the top-level crate docs that explains that this feature exists and what it does. Otherwise I'm pretty sure most people will miss it or see it and have a hard time figuring out what it does.

2 changes: 1 addition & 1 deletion validator_derive/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use darling::ast::Data;
use darling::util::{Override, WithOriginal};
use darling::FromDeriveInput;
use proc_macro_error::{abort, proc_macro_error};
use proc_macro_error2::{abort, proc_macro_error};
use quote::{quote, ToTokens};
use syn::{parse_macro_input, DeriveInput, Field, GenericParam, Path, PathArguments};

Expand Down
2 changes: 1 addition & 1 deletion validator_derive/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use once_cell::sync::Lazy;
use darling::util::Override;
use darling::{FromField, FromMeta};

use proc_macro_error::abort;
use proc_macro_error2::abort;
use quote::quote;
use syn::spanned::Spanned;
use syn::{Expr, Field, Ident, Path};
Expand Down