Recognize edition = "required" idiom

[dtolnay]

Due to #1251, I have chosen not to define a usable default edition for my workspace. See dtolnay/cxx#1035.

This PR is not required in order for the idiom in dtolnay/cxx#1035 to work, but it simply provides a better error message when some target is missing an edition attribute:

Before:

$ bazel build ...
INFO: Analyzed 48 targets (5 packages loaded, 548 targets configured).
INFO: Found 48 targets...
ERROR: /git/cxx/BUILD:4:13: Compiling Rust rlib cxx (34 files) failed: (Exit 1): process_wrapper failed: error executing command bazel-out/k8-opt-exec-2B5CBBC6/bin/external/rules_rust/util/process_wrapper/process_wrapper --arg-file bazel-out/k8-opt-exec-2B5CBBC6/bin/third-party/[email protected] --arg-file ... (remaining 28 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
error: argument for `--edition` must be one of: 2015|2018|2021. (instead was `required`)

INFO: Elapsed time: 1.251s, Critical Path: 0.14s
INFO: 2 processes: 2 internal.
FAILED: Build did NOT complete successfully

After:

$ bazel build ...
ERROR: /git/cxx/BUILD:4:13: in rust_library rule //:cxx: 
Traceback (most recent call last):
	File "/home/david/.cache/bazel/_bazel_david/ebce1d0721fb68dda9c70c0dd1405803/external/rules_rust/rust/private/rust.bzl", line 187, column 32, in _rust_library_impl
		return _rust_library_common(ctx, "rlib")
	File "/home/david/.cache/bazel/_bazel_david/ebce1d0721fb68dda9c70c0dd1405803/external/rules_rust/rust/private/rust.bzl", line 285, column 34, in _rust_library_common
		edition = get_edition(ctx, toolchain),
	File "/home/david/.cache/bazel/_bazel_david/ebce1d0721fb68dda9c70c0dd1405803/external/rules_rust/rust/private/rust.bzl", line 122, column 13, in get_edition
		fail("Attribute `edition` is required for {}.".format(ctx.label))
Error in fail: Attribute `edition` is required for //:cxx.
ERROR: Analysis of target '//:cxx' failed; build aborted: Analysis of target '//:cxx' failed
INFO: Elapsed time: 1.206s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (45 packages loaded, 1063 targets configured)

[illicitonion]

Thanks for the PR - this LGTM and I think is a nice improvement, I'll give other maintainers the rest of the day to chip in and then merge :)

[illicitonion]

Oh, @dtolnay could I bother you to fix up the lint warning by updating the attr docstring for get_edition?

[dfreese]

Instead of having a specialized string ("required"), would making default edition optional, and having None indicate this be more consistent with other optional values in the toolchain?

[UebelAndre]

One nit from me, and:

Instead of having a specialized string ("required"), would making default edition optional, and having None indicate this be more consistent with other optional values in the toolchain?

I also like this direction.

Thanks!

[dtolnay]

I updated this PR, and separately opened #1256 implementing the alternative favored by @dfreese and @UebelAndre.

[UebelAndre]

@dtolnay This one should now be ready to rebase.

[dtolnay]

I would abandon this one at this point, as #1256 has been merged. That PR was intended as an alternative to this one; #1255 (comment) says "instead of", not "in addition to".

For my personal use case, this:

rust_repositories(version = RUST_VERSION)

is gonna be just as good as:

rust_repositories(
    edition = "required",
    version = RUST_VERSION,
)

But I'd be happy to rebase if you'd find edition = "required" will be beneficial to readers.

[dtolnay]

I went ahead and rebased. I would say the value of this change is smaller after #1256 than it was originally, but perhaps still worthwhile.

[dfreese]

I don't have super strong feelings on this, so I'll let the other maintainers make the call as to whether or not this is good to add.

[dfreese]

I think the order of comparisons would need to be switched to avoid an error (None check first).

[dtolnay]

Other maintainers haven't weighed in one way or another, so I'll close because #1256 by itself covers my needs. :)