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

pvichivanives · 2024-09-06T19:56:34Z

Hi!
Given the RUSTSEC warning issued here: https://rustsec.org/advisories/RUSTSEC-2024-0370 just submitting a PR to move to the reccomended new version of procmacroerror2

Closes #351

jayvdb · 2024-09-08T21:22:16Z

Please run the tests with TRYBUILD=overwrite set.

jayvdb · 2024-09-11T05:06:46Z

ping @Keats

pvichivanives · 2024-09-12T22:02:28Z

Apparently the error messages between 1.79 and 1.81 are different. I've pushed the error messages for 1.79 since it seems thats what the CI test was failing on fist. However, one will always fail. This is true on the master branch as well.

validator_derive/Cargo.toml

Keats · 2024-09-18T15:02:57Z

validator_derive/Cargo.toml

 darling = { version = "0.20", features = ["suggestions"] }
 once_cell = "1.18.0"
+
+[features]
+nightly = ["proc-macro-error2/nightly"]


can the name be more clear somehow?

nightly_features?

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

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

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

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).

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

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.

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.

pvichivanives · 2024-10-03T23:50:28Z

Interesting, its definitely all passing locally for me except for the error message issue that I mentioned earlier. IS CI turning all feature flags to true?

Keats · 2024-10-04T12:12:35Z

It does: https://github.com/Keats/validator/blob/master/.github/workflows/ci.yml#L25-L28

pvichivanives · 2024-10-07T21:33:23Z

TY! Haven't used the github workflow before so didn't know ci was there. Updated it to only run all the feature flags. Still that issue though of the TRYBUILD=overwrite set where rn its set to 1.70. I can set it to 1.81 as well but that then we would fail. The wording was changed in 1.81. So I also bumped the tests MSRV to 1.81 but I can rever that commit if not desired.

Keats · 2024-10-11T13:37:45Z

.github/workflows/ci.yml

+          cargo build --features unic --features derive --features card
+          cargo test  --features unic --features derive --features card


What's the difference between those diffs?

This takes out the 'derive_nightly_features' feature since that doesn't work with rust stable.

If you mean between the two like between two lines not really anything

Can you add a nightly CI run that enables all the features? Otherwise that feature is never tested

Keats · 2024-10-14T17:22:22Z

That looks ok to me. Ok for everyone else as well?

pvichivanives · 2024-10-14T19:07:09Z

Wanted to note that we have an issue with Workflow though in that Nightly uses different highlighting to stable. This is expected since we are enabling the NIGHTLY warnings. Going to see if I can fix that later in another PR but for now I have try-build fail disabled with all-features

Keats · 2024-10-16T09:43:56Z

Maybe just try to build in CI using nightly rather than run the test?

pvichivanives · 2024-10-16T20:30:13Z

Happy to just build in CI too, I thought it might be more useful if we at least run all the tests we can though.

swap to proc_macro_error2

c8b5f54

trybuild=Overwrite

489e0df

pohlm01 mentioned this pull request Sep 10, 2024

Limit returned data based on user roles tweedegolf/openadr-rs#99

Merged

some stderr diff in 1.79

58cfbab

HigherOrderLogic reviewed Sep 17, 2024

View reviewed changes

validator_derive/Cargo.toml Show resolved Hide resolved

add nightly

ec3c48b

Keats reviewed Sep 18, 2024

View reviewed changes

pvichivanives added 2 commits September 18, 2024 12:03

derive nightly features

26d28c0

add Features to Readme, Changelog stuff

f32150b

add feature flags instead of all

27a37de

bump msrv to 1.81

36970c2

pvichivanives force-pushed the master branch from 71de6a4 to 36970c2 Compare October 7, 2024 21:36

pvichivanives requested a review from Keats October 10, 2024 17:19

Keats reviewed Oct 11, 2024

View reviewed changes

pvichivanives added 4 commits October 11, 2024 17:41

fix

ba4e374

use nightly toolchain?

e7668f2

fix all features

342bf45

remove extra space

c2402e9

pvichivanives added 2 commits October 14, 2024 12:02

Fix

77bc98e

fix

030cf45

disable with all-features

60e7d51

pvichivanives requested a review from Keats October 22, 2024 17:46

Keats approved these changes Oct 23, 2024

View reviewed changes

Keats merged commit b06dbc6 into Keats:master Oct 25, 2024
3 checks passed

pvichivanives mentioned this pull request Oct 29, 2024

Version changes so Rust-SEC versions can be removed #354

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pvichivanives commented Sep 6, 2024 •

edited

Loading

jayvdb commented Sep 8, 2024

jayvdb commented Sep 11, 2024

pvichivanives commented Sep 12, 2024 •

edited

Loading

Keats Sep 18, 2024

pvichivanives Sep 18, 2024

Keats Sep 19, 2024

pvichivanives Sep 19, 2024 •

edited

Loading

HigherOrderLogic Sep 20, 2024

domenicquirl Sep 27, 2024

Keats Sep 27, 2024

pvichivanives Sep 30, 2024

domenicquirl Sep 30, 2024

pvichivanives commented Oct 3, 2024 •

edited

Loading

Keats commented Oct 4, 2024

pvichivanives commented Oct 7, 2024

Keats Oct 11, 2024

pvichivanives Oct 11, 2024

Keats Oct 11, 2024

Keats commented Oct 14, 2024

pvichivanives commented Oct 14, 2024 •

edited

Loading

Keats commented Oct 16, 2024

pvichivanives commented Oct 16, 2024

		cargo build --features unic --features derive --features card
		cargo test --features unic --features derive --features card

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

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

Conversation

pvichivanives commented Sep 6, 2024 • edited Loading

jayvdb commented Sep 8, 2024

jayvdb commented Sep 11, 2024

pvichivanives commented Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvichivanives Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvichivanives commented Oct 3, 2024 • edited Loading

Keats commented Oct 4, 2024

pvichivanives commented Oct 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keats commented Oct 14, 2024

pvichivanives commented Oct 14, 2024 • edited Loading

Keats commented Oct 16, 2024

pvichivanives commented Oct 16, 2024

pvichivanives commented Sep 6, 2024 •

edited

Loading

pvichivanives commented Sep 12, 2024 •

edited

Loading

pvichivanives Sep 19, 2024 •

edited

Loading

pvichivanives commented Oct 3, 2024 •

edited

Loading

pvichivanives commented Oct 14, 2024 •

edited

Loading