softdevteam / grmtools

Rust grammar tool libraries and binaries
Other
494 stars 32 forks source link

Experiment with a cttests_macro proc_macro. #416

Closed ratmice closed 10 months ago

ratmice commented 10 months ago

This was just a quick experiment with generating tests which parse cttest/src/*.test from a proc_macro to generate #[test] fragments.

  1. Moving the cttests/build.rs helper functions to a file src/cgen_helper.rs
  2. Adds a cttests_macro crate (doesn't need to live on crates.io, used as a path dependency)
  3. Uses that macro to generate #[should_panic] tests calling the cgen_helper functions.

In #415 I know you said it probably would not be worth the effort, but I was curious if it would actually work. You had mentioned using the lang_tester crate. But I'm just uncertain of how to go about that given the build.rs involvement and (moderate) dependence on cargo environment variables like OUT_DIR within the CTBuilders.

Not really sure if all the additional complexity is worthwhile however at least I learned some proc_macro things.

ltratt commented 10 months ago

Does this mean that the tests are always run now or ... ?

ratmice commented 10 months ago

Yeah, the additions to lrpar/src/lib.rs causes it to generate a bunch of #[test] functions that run along side all of the other #[tests]. So, even if the build.rs is never rebuilt these tests will now run when you cargo test.

There is a few things in this which i should probably investigate further, regarding the globbing/lossy string conversion which might cause an issue if someone uses a non-lossy convertible character in their home directory since the manifest path is absolute. But it's a basic functioning proof of concept at how the proc-macro approach could work.

ltratt commented 10 months ago

Got it. I'm happy with this: do you want to do some of the tidying up you suggested before merge?

ratmice commented 10 months ago

I would like to tidy it up, I don't think the current master branch is affected by it so it likely is introducing a top-level cargo test regression, which could affect users with non-utf8 home directories.

It looks like both glob and globset have unresolved issues regarding this,

https://github.com/rust-lang/glob/issues/78 https://github.com/BurntSushi/ripgrep/issues/1250

In theory we can avoid non-utf8 characters by building a relative path, but it is complicated by proc_macros not having a specified working directory. Assuming it is in the workspace somewhere, we should be able to strip the common prefix between the working dir, and $CARGO_MANIFEST_DIR.

A bit hacky, but I think that is the only option short of fixing glob. Edit: I guess we could also cd $CARGO_MANIFEST_PATH && glob(...) && cd -, while that would be simplest and avoids any string manipulation it seems like it isn't thread safe, I would probably rather avoid it even though it doesn't appear that the compiler is multi-threaded during proc_macro evaluation at the moment :shrug:

ltratt commented 10 months ago

Presumably we haven't made the non-utf8 situation worse in this PR?

ratmice commented 10 months ago

Presumably we haven't made the non-utf8 situation worse in this PR?

It has or would, currently we only deal with known utf8 paths relative to the working dir. The code in this branch has to deal with 2 conversions, one in and one out of glob. The one out of glob being conversion of the path to tokens for the macro return value (I.e. there isn't a valid rust source code string which corresponds to all valid paths).

Previously we had known utf8 input, and never need to convert the output so it should be fine currently.

ratmice commented 10 months ago

I did my best to fix it in the obvious issues in that last patch, but i'm not really sure how to go about creating non-utf8 directory to test. I think though it should put us back on par with the current code on the master branch.

ltratt commented 10 months ago

I think this comes under "if the fix doesn't work for some edge cases, we'll deal with it when we find it"!

Please squash.

ratmice commented 10 months ago

Squashed.

ltratt commented 10 months ago

bors r+

bors[bot] commented 10 months ago

Build failed:

ratmice commented 10 months ago

Oops, above commit msg should be rustfmt, but given it'll be squashed away i'll leave it for now. Build failure should be fixed I believe.

ltratt commented 10 months ago

Please squash.

ratmice commented 10 months ago

Sorry. One last change to fix clippy.

ltratt commented 10 months ago

Please squash.

ratmice commented 10 months ago

Squashed -- hopefully for the last time!

ltratt commented 10 months ago

bors r+

bors[bot] commented 10 months ago

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here. For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.