Fixed inability to use `crate` attribute in wrapper rules with proc-macro targets. #807

UebelAndre · 2021-06-26T20:38:42Z

It's incorrect to totally disregard the crate type of a target passed to rust_test::crate since that info is needed to determine some flags at compile time. There may be more but this was the one I found when trying to flip the examples/proc_macro example to use edition 2018 for the default example. I was unable to build with the following issue:

error[E0432]: unresolved import `proc_macro`
  --> proc_macro/src/lib.rs:18:5
   |
18 | use proc_macro::TokenStream;
   |     ^^^^^^^^^^ use of undeclared crate or module `proc_macro`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0432`.

By keeping track of the original crate type, we can appropriately include --extern proc_macro in the linker flags for these kinds of test targets. I was able to confirm that Cargo is setting this extern flag for the same test target by copying the code from the example into a cargo workspace and adding the following diff

diff --git a/examples/proc_macro/src/lib.rs b/examples/proc_macro/src/lib.rs
index 87b9d47..08d102a 100644
--- a/examples/proc_macro/src/lib.rs
+++ b/examples/proc_macro/src/lib.rs
@@ -23,3 +23,8 @@ use proc_macro::TokenStream;
 pub fn hello_world(_input: TokenStream) -> TokenStream {
     TokenStream::new()
 }
+
+#[test]
+fn test_test() {
+    println!("test");
+}

The workspace structure was:

├── Cargo.lock
├── Cargo.toml
├── src
│   └── lib.rs
└── tests
    └── greeting.rs

2 directories, 4 files

and the results of cargo test --verbose were:

   Compiling proc_macro_lib_2018 v0.1.0 (/Users/user/Code/test/proc_macro/2018)
     Running `rustc --crate-name proc_macro_lib_2018 --edition=2018 2018/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type proc-macro --emit=dep-info,link -C prefer-dynamic -C embed-bitcode=no -C split-debuginfo=unpacked -C debuginfo=2 -C metadata=ff213938a7a3dda6 -C extra-filename=-ff213938a7a3dda6 --out-dir /Users/user/Code/test/proc_macro/target/debug/deps -C incremental=/Users/user/Code/test/proc_macro/target/debug/incremental -L dependency=/Users/user/Code/test/proc_macro/target/debug/deps --extern proc_macro`
     Running `rustc --crate-name proc_macro_lib_2018 --edition=2018 2018/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi --emit=dep-info,link -C prefer-dynamic -C embed-bitcode=no -C split-debuginfo=unpacked -C debuginfo=2 --test -C metadata=1f84dca61c8a5688 -C extra-filename=-1f84dca61c8a5688 --out-dir /Users/user/Code/test/proc_macro/target/debug/deps -C incremental=/Users/user/Code/test/proc_macro/target/debug/incremental -L dependency=/Users/user/Code/test/proc_macro/target/debug/deps --extern proc_macro`
     Running `rustc --crate-name greeting --edition=2018 2018/tests/greeting.rs --error-format=json --json=diagnostic-rendered-ansi --emit=dep-info,link -C embed-bitcode=no -C split-debuginfo=unpacked -C debuginfo=2 --test -C metadata=4b3cde5d33b9a879 -C extra-filename=-4b3cde5d33b9a879 --out-dir /Users/user/Code/test/proc_macro/target/debug/deps -C incremental=/Users/user/Code/test/proc_macro/target/debug/incremental -L dependency=/Users/user/Code/test/proc_macro/target/debug/deps --extern proc_macro_lib_2018=/Users/user/Code/test/proc_macro/target/debug/deps/libproc_macro_lib_2018-ff213938a7a3dda6.dylib`
    Finished test [unoptimized + debuginfo] target(s) in 0.78s
     Running `/Users/user/Code/test/proc_macro/target/debug/deps/proc_macro_lib_2018-1f84dca61c8a5688`

running 1 test
test test_hi ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running `/Users/user/Code/test/proc_macro/target/debug/deps/greeting-4b3cde5d33b9a879`

running 1 test
test test_hello_world_macro ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

   Doc-tests proc_macro_lib_2018
     Running `rustdoc --edition=2018 --crate-type proc-macro --crate-name proc_macro_lib_2018 --test /Users/user/Code/test/proc_macro/2018/src/lib.rs -L dependency=/Users/user/Code/test/proc_macro/target/debug/deps -L dependency=/Users/user/Code/test/proc_macro/target/debug/deps --extern proc_macro_lib_2018=/Users/user/Code/test/proc_macro/target/debug/deps/libproc_macro_lib_2018-ff213938a7a3dda6.dylib --extern proc_macro -C embed-bitcode=no --error-format human`

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

Requires:

Organized examples #803

hlopko · 2021-06-28T11:55:56Z

If it's fine with you, I'll not review this PR until Organized examples #803 is merged, the diff is huge right now. Could you please ping me here once it happens?

UebelAndre · 2021-06-28T14:10:06Z

If it's fine with you, I'll not review this PR until Organized examples #803 is merged, the diff is huge right now. Could you please ping me here once it happens?

@hlopko This is now ready for review 🙏

illicitonion

LGTM!

hlopko

Thanks for working on this! Now that I'm thinking about it I might have hit this bug in the past, I just didn't know it was in the rules.

rust/private/rustdoc.bzl

hlopko · 2021-06-29T18:31:53Z

rust/private/providers.bzl

@@ -27,6 +27,10 @@ CrateInfo = provider(
        "root": "File: The source File entrypoint to this crate, eg. lib.rs",
        "rustc_env": "Dict[String, String]: Additional `\"key\": \"value\"` environment variables to set for rustc.",
        "srcs": "depset[File]: All source Files that are part of the crate.",
+        "transitive_type": (
+            "str, optional: The original crate type for targets generated using a previously defined " +


It seems this is the only optional field in this provider, and you seem to be adding it everywhere explicitly. Are you trying to make the code backwards compatible?

I've never introduced an optional parameter on a provider before so I had assumed I needed to plug it in everywhere. But Assuming I don't set it, does it default to None?

By leaving this blank, I run into

Traceback (most recent call last): File "/Users/user/Code/rules_rust/rust/private/rust.bzl", line 182, column 32, in _rust_library_impl return _rust_library_common(ctx, "rlib") File "/Users/user/Code/rules_rust/rust/private/rust.bzl", line 253, column 32, in _rust_library_common return rustc_compile_action( File "/Users/user/Code/rules_rust/rust/private/rustc.bzl", line 533, column 36, in rustc_compile_action args, env = construct_arguments( File "/Users/user/Code/rules_rust/rust/private/rustc.bzl", line 462, column 71, in construct_arguments is_type_proc_macro = crate_info.type == "proc-macro" or crate_info.wrapped_crate_type == "proc-macro" Error: 'CrateInfo' value has no field or method 'wrapped_crate_type' Available attributes: aliases, deps, edition, is_test, name, output, proc_macro_deps, root, rustc_env, srcs, type Error: 'CrateInfo' value has no field or method 'wrapped_crate_type' Available attributes: aliases, deps, edition, is_test, name, output, proc_macro_deps, root, rustc_env, srcs, type

It seems the field needs to be explicitly created.

That is very unfortunate. This makes this a backwards incompatible change (and the field is not optional so please fix the documentation).

This could have been prevented if we had rust_common.create_crate_info function. I propose following:

let's create rust_common.create_crate_info function. I'm happy to do it if you want.

let's update the docs of rust_common.crate_info and say this is not supposed to be used for construction, only for accessing the provider (unfortunately provider constructor is used as both constructor and type declaration that we use in rule definitions or when getting providers from target).

let's call this function in all the places in this repo where we currently call rust_common.crate_info(...).

let's recommend people migrating for this PR to use create_crate_info instead of crate_info.

Was #818 what you were thinking here?

rust/private/providers.bzl

hlopko

Looking good, only minor comments and should be good to go.

rust/private/rustc.bzl

test/unit/proc_macro/proc_macro_2015.rs

hlopko · 2021-07-01T07:17:29Z

test/unit/proc_macro/proc_macro_test.bzl

+        edition = edition,
+    )
+
+    rust_test(


Please move the integration test to //test/proc_macro. Let's only keep unit tests in //test/unit/...

Why aren't they all just considered tests and go in the root of test? It feels like we're gonna exponentially gain nearly identical rust files if we need to split everything like this.

We should have order of magnitude more unit tests than integration tests. If they are duplicated, delete the integration test.

I think the thing that bothers me is that targets and unit tests are declare together. I'd like to think of unit tests as assertions on existing targets but that I could organize it such that I have a bunch of targets in test/* and then potentially only define unit test targets (which wouldn't be rust_* targets) in test/unit/**. But maybe that doesn't make sense (I struggle greatly with starlark unittests and really dislike them 😅)

Updated. But I think the coupling (decoupling) here will become unmaintainable if we continue with this pattern. I see no reason why there's a need for the test/unit directory. I would say each test/* directory defines integration tests as the base targets that are used for the relevant unit tests in the same package or a unit subpackage.

Oh I'd like to go the other way. I'm bothered that the test setup and assertions are too far away from each other (and test setup is below assertions in the file, that is also bad). Ideally I'd like to write a test like:

def test_that_yolo_is_foo(ctx): ctx.package("//foo", """ rust_library(name = "asdf", srcs = ["..."]) rust_test(name="asdf_test", crate=":asdf") """ argv = ctx.target("//foo:asdf_test").actions[0].argv assert_list_contains_adjacent_elements("--extern", "yolo")

And even more ideally this test would not execute any actions.

test/unit/proc_macro/proc_macro_test.bzl

rust/private/rustc.bzl

google-cla bot added the cla: yes label Jun 26, 2021

UebelAndre force-pushed the proc branch 2 times, most recently from 2521ee8 to 678ab71 Compare June 26, 2021 20:58

UebelAndre mentioned this pull request Jun 26, 2021

Added crate attribute to rustdoc rules (deprecated dep) #799

Merged

hlopko self-requested a review June 28, 2021 11:54

UebelAndre added 2 commits June 28, 2021 07:05

Fixed inability to use rust_test::crate with proc-macro targets.

6271388

Updated example

909db83

UebelAndre force-pushed the proc branch from 678ab71 to 909db83 Compare June 28, 2021 14:06

UebelAndre marked this pull request as ready for review June 28, 2021 14:09

illicitonion approved these changes Jun 29, 2021

View reviewed changes

Merge branch 'main' into proc

2ec8ccd

hlopko reviewed Jun 29, 2021

View reviewed changes

UebelAndre added 4 commits June 29, 2021 12:21

Renamed to transitive_type to wrapped_crate_type

9e51f35

Merge branch 'main' into proc

5303e35

Buildifier fixes

626acce

Merge branch 'main' into proc

4ed8c3a

UebelAndre requested a review from hlopko June 29, 2021 19:49

Merge branch 'main' into proc

202dfe7

UebelAndre changed the title ~~Fixed inability to use rust_test::crate with proc-macro targets.~~ Fixed inability to use crate attribute in wrapper rules with proc-macro targets. Jun 30, 2021

Added unittests

d7d3c8c

hlopko reviewed Jul 1, 2021

View reviewed changes

UebelAndre added 6 commits July 1, 2021 01:03

Updated examples and addressed feedback

cf297bf

Updated examples again

d506315

Addressed feedback

fb9aebb

Merge branch 'main' into proc

6f64be9

Fixed optional argument not being optional

ca1e68b

Addressed feedback

56c3048

hlopko approved these changes Jul 1, 2021

View reviewed changes

rust/private/rustc.bzl Outdated Show resolved Hide resolved

UebelAndre added 3 commits July 1, 2021 13:07

Moved integration tests

a094ea5

Addressed nit

fe365b2

Merge branch 'main' into proc

a237dcd

UebelAndre merged commit 01cc76b into bazelbuild:main Jul 1, 2021

UebelAndre deleted the proc branch July 1, 2021 20:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed inability to use `crate` attribute in wrapper rules with proc-macro targets. #807

Fixed inability to use `crate` attribute in wrapper rules with proc-macro targets. #807

UebelAndre commented Jun 26, 2021 •

edited

Loading

hlopko commented Jun 28, 2021

UebelAndre commented Jun 28, 2021

illicitonion left a comment

hlopko left a comment

hlopko Jun 29, 2021

UebelAndre Jun 29, 2021

UebelAndre Jun 29, 2021 •

edited

Loading

hlopko Jul 1, 2021

UebelAndre Jul 1, 2021

hlopko left a comment

hlopko Jul 1, 2021

UebelAndre Jul 1, 2021

hlopko Jul 1, 2021

UebelAndre Jul 1, 2021 •

edited

Loading

UebelAndre Jul 1, 2021

hlopko Jul 1, 2021

Fixed inability to use crate attribute in wrapper rules with proc-macro targets. #807

Fixed inability to use crate attribute in wrapper rules with proc-macro targets. #807

Conversation

UebelAndre commented Jun 26, 2021 • edited Loading

hlopko commented Jun 28, 2021

UebelAndre commented Jun 28, 2021

illicitonion left a comment

Choose a reason for hiding this comment

hlopko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UebelAndre Jun 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlopko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UebelAndre Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fixed inability to use `crate` attribute in wrapper rules with proc-macro targets. #807

Fixed inability to use `crate` attribute in wrapper rules with proc-macro targets. #807

UebelAndre commented Jun 26, 2021 •

edited

Loading

UebelAndre Jun 29, 2021 •

edited

Loading

UebelAndre Jul 1, 2021 •

edited

Loading