RFC: --crate-attr

[jyn514]

Rendered

cc rust-lang/rust#138287

[jyn514]

after thinking about this, i think this should always apply to the crate being tested. to apply to the generated test people can use --crate-attr=doc(test(attr(...))) (which is a bit of a mouthful, but, at least it's unambiguous.)

[tamird]

such behavior would deviate from the semantics of existing flags like --warn which apply to both the crate being tested and its doctests.

[jyn514]

hmm, ok. i will change the RFC to specify it should apply to both.

cc @rust-lang/rustdoc

[jyn514]

i actually expanded this into something more complicated; see the RFC for details.

[Nemo157]

From what I can tell rustdoc --test --deny=... does not apply to either the crate being tested or its doctests. rustdoc --test -Zcrate-attr seems to only apply to the generated doctests crates, so -Zcrate-attr=doc(test(attr(...))) does not currently work.

[jyn514]

both of those seem like implementation bugs to me. i am happy to open an issue on rl/r if this RFC is merged.

[epage]

One of the potential routes for build-std is that the Cargo.toml has metadata for which std crates are used which means Cargo would need to be able to tell rustc about #[no_std]. Unsure if there would have been another way to do it but this looks like it would allow that!

[jieyouxu]

Question (not necessarily for you but the Cursed Code Specialists): do we have any wonky (stable) crate-level attributes like #![crate_name = EXPR] (whose validation was only recently fixed as a breaking change in rust-lang/rust#127581) that an injection like --crate-attr=crate_name=include_str!("crate_name.txt") can lead to... interesting behaviors?

Or perhaps, can crate-root attributes take a "value" that's a macro, possibly line!()?

[jyn514]

these should be questions for the individual attributes, imo. crate-attr does not try to special-case these in any way, it just forwards the macro invocation which gets rejected later in compilation. i do not think we should try to limit the syntax to things that "look normal".

$ rustc +r2-stage1 src/main.rs -Zcrate-attr=crate_name='line!()'
error: malformed `crate_name` attribute input
 --> <crate attribute>:1:1
  |
1 | #![crate_name=line!()]
  | ^^^^^^^^^^^^^^^^^^^^^^ help: must be of the form: `#![crate_name = "name"]`

(here r2-stage1 is a checkout of rust-lang/rust#138336)

[jieyouxu]

Yeah that seems completely fair (if anything, may be QoI issue on specific crate-level attrs)

[jieyouxu]

(I'll temporarily leave this thread open in case people do know of any Funny Things, but please resolve this comment chain before FCP as it is not intended to be blocking).

[jyn514]

oh, i misunderstood. you were asking what include! and line! should expand to in this context. my understanding is that we do not currently have any crate-level attributes which accept values, right? i'm tempted to say we just delay defining this until we need to.

[fmease]

I agree with Jyn that we shouldn't restrict the grammar of those attributes.

To answer the original question however, there are several. E.g., #![doc = …], #![deprecated = …], #![type_length_limit = …]. In these three cases you can use concat!("", line!()) as the RHS which expands to (lineno) "1" when passed via -Zcrate-attr (I'm using concat to essentially get rid of the numeric suffix).

(crate_name, crate_type and recursion_limit are fully locked-down nowadays in the sense that they no longer accept macro calls)

(since a lot of malformed and misplaced attrs are still accepted by rustc and only trigger the warn-by-default lint unused_attributes, you can currently also do things like #![must_use = compile_error!("…")] which isn't that interesting arguably)

[Nemo157]

rustdoc -Zcrate-attr='doc=concat!("test", line!())' foo.rs gave a doc/foo/index.html containing "test1".

[jyn514]

ok. of these, line!() really does not seem to have a meaning to me, maybe the implementation comes up with "1" but i don't think that's useful. so i'm tempted to ban that particular macro in this context.

include! and friends all seem fine to me, they should be relative to whatever #![doc = include_str!()] in the source is relative to.

[jieyouxu]

FWIW I was mostly thinking adversarially on what weird combinations this can end up with (e.g. what's prone to break, between a stable + stable feature/functionality interaction). I suppose this may also include the other "meta" ones like column!(). Not sure how to phrase this, but "observable implementation details" -- e.g. that line number 1 being observable through line!() is potentially interesting if it can become depended upon by the user.

Alternatively... explicitly carve out a stability caution/exception where it's explicitly remarked that depending on line!()/column!() etc. is not part of the stability guarantees? Idk how to word this though. It's not a big concern but it can be annoying if it ever comes up.

[jyn514]

i still think that all these concerns apply just as much to existing crate-level attributes in source code. if there is a concern here it should be specific to attributes passed via a CLI flag.

[jieyouxu]

Discussion: maybe worth explicitly calling out the CLI order in which --crate-attr flags are specified is important too (and that they are not commutative or whatever you call it), e.g. given a source file

// foo.rs
#![warn(unused_must_use)]

fn main() {}

rustc invocation:

rustc --crate-attr=deny(unused_must_use) --crate-attr=allow(unused_must_use) foo.rs

then the #![warn(unused_must_use)] wins since it's the last lint level of unused_must_use on the crate-root node 🤔

[jyn514]

so, it turns out i actually documented this wrong. the attributes are appended to the crate attrs in the source. what that means is crate-attr takes precedence over attributes in the source:

; head -n2 src/main.rs
#![warn(unused)]
fn foo() {}
; rustc +r2-stage1 src/main.rs -Zcrate-attr='allow(unused)' -Zcrate-attr='deny(unused)'

error: function `foo` is never used
 --> src/main.rs:4:4
  |
4 | fn foo() {}
  |    ^^^
  |
note: the lint level is defined here
 --> <crate attribute>:1:9
  |
1 | #![deny(unused)]
  |         ^^^^^^

this is unfortunate because it means that crate-attr=allow is not equivalent to -A (right now, the former takes precedence over the source, the latter does not).

i am tempted to say this is "just" a bug in an implementation and it should match the docs.

[jieyouxu]

I can see why this order makes sense, but also I feel like --crate-attr taking precedence (or rather, get applied first) is "more useful" for actual users.

[jyn514]

i am not sure what you mean by "this order". are you saying CLI attrs should be prepended or appended to source attrs?

[kpreid]

For the stated use case of “CLI flags are easier to configure for a whole workspace at once”, prepending is importantly more flexible because it allows crates to override the global configuration.

[jieyouxu]

Ah I mean I can see why the current implementation order (appended) might be useful for a scenario where you want the cli crate-level attributes to come after the in-source crate-level attributes (append-ordering) and thus take precedence over the in-source crate-level attributes. This is a very contrived example, but e.g.:

$ rustc --crate-attr='cfg_attr(target_os = "windows", allow(missing_docs))' foo.rs

#![warn(missing_docs)]
fn main() {}

With the append-ordering, you can use --crate-attr to suppress the lint but only if the target_os is windows.

(I'm personally in favor of the prepend-ordering, though it may be worth calling out this as an potential alternative behavior design choice.)

[jyn514]

it sounds like we've settled on the prepend ordering, but i want to call out explicitly: the point of this RFC is not for lints. there are already many ways to configure lints, including one explicitly supported by cargo. although i am documenting the behavior here, my hope is that no one should ever need to know or care about the order these are applied.

[jieyouxu]

Indeed, my intention is not that this is about lints or lint levels. I'm only using lints as a cherry-picked example since that's the easiest to play with. My intention was that we should explicitly document that there is indeed an alternative design choice (append-ordering).

[jieyouxu]

Remark: although it does visually look closer to what the user would've wrote in the
source file.

[jieyouxu]

Remark: +1, I think it's perfectly fine (and arguably good) to not support multi-attr in same flag. If anything, multiple --crate-attr flags is way easier to read too, IMO.

[kennytm]

does this mean --crate-attr and -A/-D/-W/-F can combine with the same priority i.e. the last one wins?

it seems the current implementation is actually "first apply all the -A/-D/-W/-F, then apply all the -Zcrate-attr".

# currently equivalent to -A unused
rustc -W unused -A unused 1.rs 
# currently equivalent to -W unused
rustc -A unused -W unused 1.rs 
# proposed to be equivalent to -A unused
rustc --crate-attr 'warn(unused)' --crate-attr 'allow(unused)' 1.rs

# does this become -A unused ? (on nightly's -Zcrate-attr it is equivalent to -W unused)
rustc --crate-attr 'warn(unused)' -A unused 1.rs
# does this become -A unused ? 
rustc -W unused --crate-attr 'allow(unused)' 1.rs
# does this become -D unused ? (on nightly's -Zcrate-attr it is equivalent to -A unused)
rustc -W unused --crate-attr 'allow(unused)' -D unused 1.rs
# does this become -D unused ?
rustc --crate-attr 'warn(unused)' -A unused --crate-attr 'deny(unused)' 1.rs

[jyn514]

oh, this is a good callout. i was imagining the semantics to be that -A/W/D flags are parsed before crate attrs are injected, which groups your examples the way that nightly's -Zcrate-attr currently does. but it would be cute to make A/W/D an alias for crate-attr, such that we can remove that flag processing in the compiler almost altogether and have 'warn(unused)' -A unused mean -A unused. i'm in favor of that, i don't want to add yet more precedence grouping to lints.