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.
rustdoc-json: Output target feature information #139393
New 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
rustdoc-json: Output target feature information #139393
Changes from all commits
8c50f95
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.
I assume:
If all this is true, perhaps we should note it in the doc comment. If not, we should definitely note that it isn't true :)
The first two items can be made obvious by making this set instead of a
Vec
, though I'm ambivalent on whether that's an improvement or not. Your call!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.
This all should be true, but it's not ensured by the underlying data structures.
rustc_target::target_features::ImpliedFeatures
is just a&'static [&'static str]
. Implying nonexistent features or forming a cycle wouldn't cause problems unless that feature were enabled.If we're providing these guarantees, I'd be inclined to have
librustdoc
verify andpanic!()
ifrustc_target
tries to violate them. Is adding this check (and failure mode) desirable? If not, should we document these expectations with weaker language?Separately, I note that
rustdoc-json-types
does not currently use any sets, onlyVec
s. It uses and re-exportsrustc_hash::FxHashMap
for maps. Should I re-exportFxHashSet
too?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 defer to rustdoc JSON maintainers on whether
librustdoc
should verify the guarantees explicitly.From my point of view, it's fine to expose either a
Vec
orFxHashSet
here.If the properties are expected but not able to be guaranteed, I'd also be in favor of documenting them with weaker language.
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 think this should be a
Vec
, as we use that in other places inrustdoc_json_types
for things where the order doesn't matter (egModule::items
,Struct::impls
).But maybe we should separately think about if we want to only use
Vec
where the order does matter, and some set type for where it doesn't.As for testing the validity of features, that shouldn't live in rustdoc, but in rustc_target (and done in a separate PR). What guarantees they make are up to T-compiler (and maybe T-lang??) and if their broken, it'd be much nicer to get the error from compiler tests than rustdoc tests.
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 don't have strong feelings either way on
Vec
vs set type for places where order doesn't matter. If the expectation (from docs or from context) is that order is irrelevant, my preference would be "whatever is faster to iterate over" so probably slight preference toward aVec
overall :)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 checking my understanding of the implication here: the set of features with
globally_enabled = true
lets us figure out what#[cfg]
'd items may be enabled or removed in the rustdoc run that produced this file, right?No change needed if that's the case, I just wanted to make sure there's no additional hidden meaning behind
globally
orsession
.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 chose
globally
to indicate that this feature is enabled "everywhere", as opposed to "here in thisfn
", but it seems strictly more correct to say "everywhere in this session". I'm not sure whether this distinction is important. Are there circumstances where different parts of the same build can have different-Ctarget-feature
or-Ctarget-cpu
flags?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'm not sure either — this is also why I was asking :)
I'm not familiar with the specific definition of a "session" either so it might be good to try to work out more precise language, together with the rustdoc JSON maintainers.
I'd be perfectly happy with a TODO on that more precise language though — what this PR has is certainly a large improvement over the status quo and would be valuable to have already.
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.
Maybe something along the lines of "in this compilation"/"in this rustdoc invocation". I think saying "session" is precise, but it isn't accessible, as it's not used in a user facing way.
(I agree it'd be fine to clear this up later)