softdevteam / grmtools

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

Clippy fallout & overzealous #[deny(unsafe_code)] #427

Closed ratmice closed 7 months ago

ratmice commented 7 months ago

The first patch is just fixing normal clippy stuff.

In the second patch clippy was suggesting that we remove the unsafe code because it (being just a test) doesn't actually do anything. Rather than #[allow(...)] I tried defining an unsafe function in the program section and call that.

Turns out #[deny(unsafe_code)] is still enabled in the program section, since my patch at https://github.com/softdevteam/grmtools/pull/370

Subsequently the second patch writes the combined user actions + program section into a single module, and #![allow(unsafe_code)] within that module. Previously the program() section could not specify inner attributes since those must be specified at the top of a module, so they couldn't easily opt in using #![allow(unsafe_code)]. With this reorganization it is now possible that the program section is at the top of the module, and can specify an inner attribute.

user actions + program code, need to be in the same module, because we currently don't require pub visibility modifiers between them.

Edit: The other option being we could just revert the #[deny(unsafe_code)] part from the generated sources, WDYT?

ratmice commented 7 months ago

Did we lose the rustfmt check?

ltratt commented 7 months ago

Please squash.

ltratt commented 7 months ago

Did we lose the rustfmt check?

It's still part of .buildbot.sh but we now use github's merge queues to kick buildbot off, rather than bors.

ratmice commented 7 months ago

Squashed.

ltratt commented 7 months ago

@ratmice If you click on "details" then "buildbot/buildbot-script" you can see the CI log and what went wrong. Hopefully it's not too annoying to fix!

ratmice commented 7 months ago

I think I just need to readd the pub to pub use _user_section_::*, however I'm very curious why I don't seem to be able to reproduce locally.

Edit: reproduced locally just needed to actually be in that example, thought --all-targets covered examples.

ratmice commented 7 months ago

So that fixed one of the issues, but the other might be insurmountable. calc_actions/src/calc.y doesn't re-export it's use std::error::Error without changing it to pub use std::error::Error.

I'll have to have a closer look, but it definitely might sink this approach.

ratmice commented 7 months ago

So, I basically had to flip it inside out, moving the stuff that had #![deny(unsafe_code)] to it's own module instead.

Edit: Now that I have committed this, I do have one concern with allowing inner attributes. The concern is that inner attributes will also apply to generated parse code (in the parser module).

Unfortunately we can't really fix that (to my knowledge), because it relies upon the specific behavior unique to use super::*, which imports use statements from the parent module. Trying to separate them in into sibling modules runs into that issue where mod X { use std::error::Error } mod Y { use super::X::*; fn foo() -> Box<dyn Error> } has no Error import.

So we may not want to fully guarantee that inner attributes can be specified in the user program section.

ltratt commented 7 months ago

Excellent! Please squash.

ratmice commented 7 months ago

Squashed, I rewrote the commit message a bit to mention the perils of using #![inner_attribute] in the program section though.

ltratt commented 7 months ago

Thanks!