Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RFC:
--crate-attr
#3791New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
RFC:
--crate-attr
#3791Changes from all commits
9d7b2f6
ed9d231
cec9178
c29d394
7b03a05
aca698c
046463b
4b9fa4b
69df2b8
f1cb06b
640136e
8ef1b64
9a9b8da
b213d1f
4c1b2ba
a3abbbe
cb4205d
327a5ec
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, register-tool would have to be configured in the workspace -- passing it only when the tool was invoked likely doesn't work since then the "passive" state of the code has a bunch of non-registered attributes? Maybe I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as a note, RfL already does this exact thing and they would like to see
-Zcrate-attr
stabilized as well:https://github.com/Rust-for-Linux/linux/blob/e6ea10d5dbe082c54add289b44f08c9fcfe658af/scripts/Makefile.build#L238-L239
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!()
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".(here
r2-stage1
is a checkout of rust-lang/rust#138336)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that seems completely fair (if anything, may be QoI issue on specific crate-level attrs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, i misunderstood. you were asking what
include!
andline!
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 useconcat!("", 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
andrecursion_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)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rustdoc -Zcrate-attr='doc=concat!("test", line!())' foo.rs
gave adoc/foo/index.html
containing "test1".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 number1
being observable throughline!()
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I don't think this quite matches how expansion actually works -- in particular step 1 in the compiler can't parse the full crate, since that's inherently interleaved with macro expansion. But I don't think that fundamentally changes anything specified here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, it might be a good idea to specify this feature in context of precise operation of today's expansion if only to get ahead of potential underspecification issues if the details of macro expansion were to change in the future. I don't expect this to change the structure much, but it may require author to give a closer read on the parser's code.
(I'd be also fine with this part of RFC being refined to describe what ends up being implemented later down the line…)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to mention we already have
@file
to pass in arguments from a file if for some reason it is necessary to have a single argument in the list of arguments.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both
line!
andcolumn!
should refer to the original source code, as written (i.e. prior to expansion.) That's their current behaviour, isn't it? I see no reason why that ought to change. Any reference to these macros would expand to the first column of the first line, regardless of the number of attributes or their length in that case.Why? Consider the primary use-case for these macros – inserting location info into debug messages. It'd be weird if the logs said that the location is at line 100, but the expression that produced the log line ends up being on line 95 (all because 5 attributes have been introduced.) Same thing stands for
column!
(there's nothing preventing one from typing out the entire rust module into a single line.) Could argue that the actual source-codeline!
s andcolumns!
could retain their behaviour still, but at that point we end up with a weird situation where the sameline:col
could refer to two entirely distinct locations.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem presented here is the span of a
line!
inside a--crate-attr
. So like a--crate-attr doc(concat!("i'm on: ", line!()))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that. The load bearing part in my comment is the 2nd paragraph. Let me try rephrasing it.
If any non-trivial logic had to be prescribed to
line!()
andcolumn!()
inside these inserted attributes there are two ways to go about doing so:line!()
and/orcolumn!()
(thus breaking basic expectations of these macros); orfile:line:col
refers to exactly one location in the source.Neither of these options are viable in my opinion. 1st, again, breaks basic expectations of these macros. As for 2nd option consider a situation where a diagnostic has to be shown at, say line 1 column 8~16. Does rustc display a snippet from an introduced attribute? A snippet from the source file before attributes were introduced? Maybe rustc isn't a great example, as error could be in either of these locations & rustc's source location tracking will need to be adjusted regardless. But what about other tools? Perhaps even tools that don't necessarily have any business parsing rust code (and thus no reason to accept
--crate-attr
as an argument,) but need to tie lines and columns back to the original source somehow?These considerations ultimately lead to only one sensible implementation for
line!()
/column!()
in introduced attributes: they both should result in0
,1
or some similar placeholder of some sort that proper use ofline!()
/column!()
cannot otherwise produce. Such approach can also be rationalized by describing attribute insertion as:Since macro invocations present a
#[track_location]
-like behaviour, allline!()
andcolumn!()
invocations thatinsert_attributes!()
expands to produce the same value (in this case0:0
,1:1
or whatever else we pick.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just do what clang does and consider the inserted attributes to be in the file
<built-in>
or similar?https://gcc.godbolt.org/z/aYn3cz5c3
clang++ ... --include=cstddef --include=unknown2
gives: