Rewrite style.rs using syn

[rmehri01]

Description

• Use syn instead of text-based parsing since it's more accurate and easier to work with
• Move style.rs to libc-test and make it run as a part of cargo test, as well as cleaning it up a bit and adding tests for the style checker implementation itself
• Use a visitor that will walk over all the items in the file, including within cfg_if! macros, and tries to apply the same logic that the previous style.rs script did
• Fix some lints that came up as a result of rewriting the script
• Added some new rules for dealing with s! macros

Closes #4109

Sources

Checklist

• Relevant tests in libc-test/semver have been updated
• No placeholder or unstable values like *LAST or *MAX are
  included (see #3131)
• Tested locally (cd libc-test && cargo test --target mytarget);
  especially relevant for platforms that may not be checked in CI

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @tgross35 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[tgross35]

Huge thanks for working on this!! I’ll take a look in a couple of days :)

[tgross35]

Sorry that took a little longer than expected. Overall this looks great, most of the comments I left relate to things we can probably upgrade now since the original is from 2016.

One meta thing - maybe it's better to put this into a new module of libc-test/src/ and then only run it in libc-test/test. Then you could add a new test file to check this implementation itself, just have a bunch of examples in strings and assert they pass/fail.

[rmehri01]

One meta thing - maybe it's better to put this into a new module of libc-test/src/ and then only run it in libc-test/test. Then you could add a new test file to check this implementation itself, just have a bunch of examples in strings and assert they pass/fail.

I couldn't quite get it to work with libc-test/src/ since there were some linking errors, I think having to do with using cc in the build script. For now I just moved the common functionality into tests/style/mod.rs similar to what is suggested here so that we can test the style checker implementation as well: https://doc.rust-lang.org/rust-by-example/testing/integration_testing.html.

[rmehri01]

Sorry for cramming so much into one pr but I think I came up with some decent rules for doing the #[cfg(...)] checking, basically just checking if there are duplicates and standalone positive #[cfg(...)] attributes instead of requiring that cfg_if! is used everywhere:

libc/libc-test/test/style/mod.rs

Lines 22 to 26 in 60c840e

	//! * Multiple s! macros are allowed as long as there isn't a duplicate cfg,
	//! whether as a standalone attribute (#[cfg]) or in a cfg_if!
	//! * s! macros should not just have a positive cfg since they should
	//! just go into the relevant file but combined cfgs with all(...) and
	//! any(...) are allowed

Here are some examples, mostly taken from what you mentioned in the issue/review comments:

libc/libc-test/test/style_tests.rs

Lines 135 to 260 in 60c840e

	#[test]
	fn accept_multiple_s_macros_with_disjoint_cfg() {
	let contents = r#"
	// Main `s!`
	s! {}
	// These are not supported on a single arch. It doesn't make sense to
	// duplicate `foo` into every single file except one, so allow this here.
	#[cfg(not(target_arch = "foo"))]
	s! { pub struct foo { /* ... */ } }
	// Similar to the above, no problems here
	#[cfg(not(target_os = "illumos"))]
	s! { pub struct bar { /* ... */ } }
	"#
	.to_string();
	let mut checker = StyleChecker::new();
	checker.check_string(contents).unwrap();
	checker.finalize().unwrap();
	}
	#[test]
	fn reject_duplicated_s_macro() {
	let contents = r#"
	s! {}
	s! {}
	"#
	.to_string();
	let mut checker = StyleChecker::new();
	checker.check_string(contents).unwrap();
	assert!(checker.finalize().is_err());
	}
	#[test]
	fn reject_duplicated_s_macro_cfg() {
	let contents = r#"
	#[cfg(not(target_arch = "foo"))]
	s! {}
	#[cfg(not(target_arch = "foo"))]
	s! {}
	"#
	.to_string();
	let mut checker = StyleChecker::new();
	checker.check_string(contents).unwrap();
	assert!(checker.finalize().is_err());
	}
	#[test]
	fn reject_single_positive_s_macro_cfg() {
	let contents = r#"
	// A positive (no `not`) config: reject because this should go into
	// the relevant file.
	#[cfg(target_arch = "foo")]
	s! { pub struct foo { /* ... */ } }
	"#
	.to_string();
	let mut checker = StyleChecker::new();
	checker.check_string(contents).unwrap();
	assert!(checker.finalize().is_err());
	}
	#[test]
	fn reject_single_positive_s_macro_cfg_target_os() {
	let contents = r#"
	// A positive (no `not`) config: reject because this should go into
	// the relevant file.
	#[cfg(target_os = "foo")]
	s! { pub struct foo { /* ... */ } }
	"#
	.to_string();
	let mut checker = StyleChecker::new();
	checker.check_string(contents).unwrap();
	assert!(checker.finalize().is_err());
	}
	#[test]
	fn accept_positive_s_macro_any() {
	let contents = r#"
	// It's nicer to accept this so that we don't have to duplicate the same struct 3 times.
	#[cfg(any(target_arch = "foo", target_arch = "bar", target_arch = "baz"))]
	s! { pub struct foo { /* ... */ } }
	"#
	.to_string();
	let mut checker = StyleChecker::new();
	checker.check_string(contents).unwrap();
	checker.finalize().unwrap();
	}
	#[test]
	fn accept_positive_s_macro_all() {
	let contents = r#"
	#[cfg(all(target_arch = "foo", target_arch = "bar", target_arch = "baz"))]
	s! { pub struct foo { /* ... */ } }
	"#
	.to_string();
	let mut checker = StyleChecker::new();
	checker.check_string(contents).unwrap();
	checker.finalize().unwrap();
	}
	#[test]
	fn reject_duplicated_cfg_and_cfg_if() {
	let contents = r#"
	#[cfg(not(target_arch = "foo"))]
	s! { pub struct foo { /* ... */ } }
	cfg_if! {
	if #[cfg(not(target_arch = "foo"))] {
	s!{ pub struct bar {} }
	}
	}
	"#
	.to_string();
	let mut checker = StyleChecker::new();
	checker.check_string(contents).unwrap();
	assert!(checker.finalize().is_err());
	}

[tgross35]

Sorry this took a little while, usually I sweep this repo about once a week. Overall, looks great! I left a handful of mostly stylistic suggestions, nothing major.

After you make any changes please just clean up the history and we should be able to get this in.

I couldn't quite get it to work with libc-test/src/ since there were some linking errors, I think having to do with using cc in the build script. For now I just moved the common functionality into tests/style/mod.rs similar to what is suggested here so that we can test the style checker implementation as well: https://doc.rust-lang.org/rust-by-example/testing/integration_testing.html.

A solution for this would probably be to make a ctest feature that is enabled by default and turns on cc, then for these style tests we just run with --no-default-features. But that's a change for later (I think the test library needs some refactoring anyway).

Sorry for cramming so much into one pr but I think I came up with some decent rules for doing the #[cfg(...)] checking, basically just checking if there are duplicates and standalone positive #[cfg(...)] attributes instead of requiring that cfg_if! is used everywhere:

Thanks for getting this feature in! It seems reasonable to me and, more importantly, it seems straightforward to adjust if needed.

[rmehri01]

Sorry this took a little while, usually I sweep this repo about once a week. Overall, looks great! I left a handful of mostly stylistic suggestions, nothing major.

After you make any changes please just clean up the history and we should be able to get this in.

No problem at all! I just addressed the comments and cleaned up the history :)

[tgross35]

I'm not quite sure why emscripten is failing, seems like it's failing to run the C build? I don't know why that would be, seems like it should be unrelated.

[tgross35]

In any case lgtm, thanks for getting this done! I'll try to figure out if our emscripten CI just broke somehow, unless you have any ideas.

[rmehri01]

I'm not quite sure why emscripten is failing, seems like it's failing to run the C build? I don't know why that would be, seems like it should be unrelated.

Yeah I wasn't sure about this either, it was passing earlier and I don't think addressing the review comments should have touched anything related to the build script.

[rmehri01]

Just reran it and it passes now so maybe it was a flake?

[tgross35]

Just reran it and it passes now so maybe it was a flake?

That's lucky 🎉. Thanks again for making this happen!