Add support for #[cfg(test)] attribute

[rw1nkler]

Description

This commit adds a #[cfg(test)] attribute that you can use to mark functions or procs that are only meant for testing. When marked this way, they won't be lowered to IR. The original request in #1524 was just for functions, but I applied the same idea to procs as well.

Implementation details

This feature uses a test_only flag in the Function class to mark functions and procs that are only used for tests. When converting code to IR, functions and procs marked as test-only are skipped, unless the convert_tests option is enabled. This allows test helpers to be included in the IR if needed.

IR conversion now provides clearer error messages. If a test-only function or proc is selected for conversion without the convert_tests option, a dedicated error is shown. Same happens when a test-only function is called from a regular function or proc and IR is being dumped.

In DSLX, the interpreter will fail if a test-only function is called from a regular one. This check is not yet in place for procs, as communication between regular and test-only procs involves more complexity, but I can add it too if needed.

Resolves #1524

[rw1nkler]

FYI @richmckeever @dplassgit

[proppy]

@rw1nkler can you add DSLX examples w/ tests functions?

[proppy]

#[cfg(test)]

This does seems slightly inconsistent w/ our current annotations #[test] and #[test_proc], should we switch everything to #[cfg(something)] or add something like #[test_aux/helper] and #[test_proc_aux/helper].

[allight]

#[cfg(test)]

This does seems slightly inconsistent w/ our current annotations #[test] and #[test_proc], should we switch everything to #[cfg(something)] or add something like #[test_aux/helper] and #[test_proc_aux/helper].

This is consistent with how rust does things (sort of, #[cfg(whatever)] acts more like a #ifdef and style is to place it on the 'tests' module but its pretty close): rust docs

[proppy]

This is consistent with how rust does things (sort of, #[cfg(whatever)] acts more like a #ifdef and style is to place it on the 'tests' module but its pretty close): rust docs

Ah you're right that's close to what Rust does:

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn it_works() {
        let result = add(2, 2);
        assert_eq!(result, 4);
    }
}

I guess one difference is that because they have mod they can enable cfg[test) for a whole hierarchy and don't have to annotate everything single helper functions/procs.

Maybe worth logging as a issue if we want something similar as a follow-up.

[cdleary]

Looks good, is there a test for what happens when we refer to an entity that's test-only though in a non-test scenario? Seems like it'd be nicer to make a DSLX level error message instead of having a linker-style error at IR conversion/opt time.

[rw1nkler]

Looks good, is there a test for what happens when we refer to an entity that's test-only though in a non-test scenario? Seems like it'd be nicer to make a DSLX level error message instead of having a linker-style error at IR conversion/opt time.

There's a test that checks if a test utility function was called from a regular function or proc:
https://github.com/google/xls/pull/2168/files#diff-c24b2f53453f91215eec0d6d76daf6a9e03f9c780d0f9710472388af1fe1ce11R1686

Right now, there are no checks for spawning utility procs inside regular procs.
If we want to catch those cases too, I can work on adding that.

[richmckeever]

It might be good to expose an inline constexpr std::string_view from ast_utils.h that is the "cfg(test)" string, so that in real code we don't have to repeat it in different places (it's fine to repeat it in test DSLX).

[rw1nkler]

I added the constant

[mikex-oss]

FYI @rw1nkler looks like you'll need to rebase to get this in.

[dplassgit]

The proc version inserts the whole attribute at the same time. Is there a benefit either way?

[dplassgit]

If at all possible this should be done in the constructor instead of in a mutator

[rw1nkler]

I wanted to be consistent with other attributes like dslx_format_disable or extern_verilog. If using a constructor is preferred I can change that.

[dplassgit]

Yes, having the bit in the constructor is preferred. The other mutator methods are only present out of necessity due to quirks in the parser.