trailofbits / dylint

Run Rust lints from dynamic libraries
https://blog.trailofbits.com/2021/11/09/write-rust-lints-without-forking-clippy/
Apache License 2.0
366 stars 21 forks source link

add cargo bless functionalty #1091

Open lucarlig opened 6 months ago

lucarlig commented 6 months ago

I have been using dylint quite a lot, the cargo bless functionality would save so much repetitive work copying and pasting.

lucarlig commented 6 months ago

maker seems to make it work with a wrapper around uitests.

smoelius commented 6 months ago

Thank you very much for your feedback. I will look into this.

lucarlig commented 6 months ago

also a better diff when running the test would be so much appreciated I opened this https://github.com/Manishearth/compiletest-rs/issues/284 but @Manishearth mentioned that the crate is not actively maintained so no luck with this. Let me know if you make a plan to improve UI testing for dylint and how I can help.

smoelius commented 6 months ago

Do you know if migrating dylint_testing from compiletest to ui_test would allow for better diffs?

lucarlig commented 6 months ago

doesn't seem to allow it, maybe could be added but the default one is better than compiletest already. I think rust really needs a better UI test framework...

lucarlig commented 6 months ago

https://github.com/oli-obk/ui_test/issues/205

smoelius commented 6 months ago

Generally speaking, do you know how difficult it is for a package to switch from using compiletest to ui_test?

lucarlig commented 6 months ago

no idea, but I can give it a try.

smoelius commented 6 months ago

That would help me out a lot!

lucarlig commented 6 months ago

will try this next week @smoelius

smoelius commented 6 months ago

Thanks!

lucarlig commented 6 months ago

i tried something like this

    let mut rustc_flags: Vec<OsString> = config.rustc_flags.iter().map(OsString::from).collect();
    rustc_flags.push(OsString::from("--emit=metadata"));
    if cfg!(feature = "deny_warnings") {
        rustc_flags.push(OsString::from("-Dwarnings"));
    }
    rustc_flags.push(OsString::from("-Zui-testing"));

    let program = ui_test::CommandBuilder {
        args: rustc_flags,
        ..ui_test::CommandBuilder::cmd(driver.to_path_buf())
    };
    let config = ui_test::Config {
        program,
        ..ui_test::Config::rustc(src_base.to_path_buf())
    };
    ui_test::run_tests(config).unwrap();

instead of compile_tests, issue is the args order doesn't work with driver, it's expecting the file path to be first.

smoelius commented 6 months ago

Thank you very much for looking into this.

instead of compile_tests, issue is the args order doesn't work with driver, it's expecting the file path to be first.

I'm not quite following. Could you post the error message?

lucarlig commented 6 months ago

this is how it's passing the command:

command: "~/.dylint_drivers/nightly-2024-03-02-aarch64-apple-darwin/dylint-driver" "--emit=metadata" "-Zui-testing" "/private/var/folders/qb/g3v983vx5px1qx4bhcwqx3700000gn/T/.tmpbiKj0k/ui/main.rs" "--edition" "2021"

the issue is dylint-driver seems to expect as first arg the filename instead is getting "--emit=metadata" ui_tests always set the filename as last

smoelius commented 6 months ago

Is it worth opening a WIP PR for what you have done?

lucarlig commented 6 months ago

probably not, it is just literally that code I posted above.