Repository navigation
Move #[doc(inline)] and #[doc(no_inline)] validation into rustc_attr_parsing - #163673
Conversation
|
Some changes occurred in compiler/rustc_passes/src/check_attr.rs cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_attr_parsing |
|
r? @oli-obk rustbot has assigned @oli-obk. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| @@ -367,13 +367,14 @@ pub(crate) fn run_global_ctxt( | |||
|
|
|||
| // NOTE: These are copy/pasted from typeck/lib.rs and should be kept in sync with those changes. | |||
| tcx.sess.time("wf_checking", || tcx.ensure_ok().check_type_wf(())); | |||
| tcx.sess.time("check_mod_attrs", || { | |||
There was a problem hiding this comment.
Had to move this before abort_if_errors. Since conflicting attributes are caught during lowering now, rustdoc was exiting early and swallowing the other errors in invalid-doc-attr.rs.
There was a problem hiding this comment.
@GuillaumeGomez could you confirm this is ok? I'm not familiar with why this abort_if_errors is there
There was a problem hiding this comment.
I'm surprised this change is needed. If the rustdoc tests pass and the perf check is happy, then so am I.
There was a problem hiding this comment.
Just double-checked and yes, seems ok. We'll wait for tests to confirm but seems fine.
| } | ||
| } | ||
|
|
||
| fn deferred_finalize_check(&self) -> Option<(FinalizeCheckFn, Span)> { |
There was a problem hiding this comment.
Deferred this check since we need the resolved target from FinalizeCheckContext to check if it's actually a use item.
There was a problem hiding this comment.
Is the resolved target needed? I don't see where you're using it
There was a problem hiding this comment.
Sorry, I confused the item’s syntactic target with the resolved import target. This only checks Use or ExternCrate, which FinalizeContext already provides. I’ll move it into finalize.
There was a problem hiding this comment.
You can also consider putting it in parse_inline
There was a problem hiding this comment.
Moved the conflict check into parse_inline and made inline an Option. The target check is still deferred since finalize takes an immutable context. That also avoids an extra target lint when the attributes already conflict.
There was a problem hiding this comment.
I think you should also just be able to put the target check in the inline parser right?
There was a problem hiding this comment.
Yes, AcceptContext already has the target and can emit the lint. I’ll move that check into parse_inline and remove the deferred callback.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
r? me |
| } | ||
| } | ||
|
|
||
| fn finalize_check(cx: &mut FinalizeCheckContext<'_, '_>, _attr_span: Span) { |
There was a problem hiding this comment.
Can we move this logic to the inline parser?
Then we can solve this comment and make inline an Option
| @@ -367,13 +367,14 @@ pub(crate) fn run_global_ctxt( | |||
|
|
|||
| // NOTE: These are copy/pasted from typeck/lib.rs and should be kept in sync with those changes. | |||
| tcx.sess.time("wf_checking", || tcx.ensure_ok().check_type_wf(())); | |||
| tcx.sess.time("check_mod_attrs", || { | |||
There was a problem hiding this comment.
@GuillaumeGomez could you confirm this is ok? I'm not familiar with why this abort_if_errors is there
|
Reminder, once the PR becomes ready for a review, use |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Move #[doc(inline)] and #[doc(no_inline)] validation into rustc_attr_parsing
|
Actually it's completely unneeded. Sorry for the noise. ^^' @bors try- |
|
Unknown command "try-". Run |
This comment has been minimized.
This comment has been minimized.
|
Some changes occurred in compiler/rustc_attr_ir |
|
@rustbot ready |
| pub(crate) struct DocParser { | ||
| attribute: DocAttribute, | ||
| nb_doc_attrs: usize, | ||
| inline_conflict: bool, |
There was a problem hiding this comment.
Is this needed?
I'd lean toward emitting multiple diagnostics if there are multiple wrong attributes being fine
There was a problem hiding this comment.
Moved the target check into parse_inline and removed the conflict flag and deferred callback. It now reports multiple diagnostics when multiple attributes are invalid, and I’ve updated the tests accordingly.
@rustbot ready
|
Finished benchmarking commit (61e9eef): comparison URL. Overall result: no relevant changes - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (secondary 1.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.2%, secondary 3.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 490.152s -> 492.034s (0.38%) |
…, r=JonathanBrouwer Move #[doc(inline)] and #[doc(no_inline)] validation into rustc_attr_parsing Part of rust-lang#153101 This migrates `#[doc(inline)]` and `#[doc(no_inline)]` validation from `rustc_passes` into `DocParser` in `rustc_attr_parsing`. It checks for conflicting inlining attributes and non-use targets, and tweaks rustdoc's early abort so lowering errors don't suppress other attribute diagnostics, also added new UI tests in doc-inline-validation.rs.
…uwer Rollup of 4 pull requests Successful merges: - #160915 ([rustdoc] Fix how `Deref` items is handled.) - #163267 (fix `VisibleForLeakCheck` in `RegionOutlives` fast path) - #163673 (Move #[doc(inline)] and #[doc(no_inline)] validation into rustc_attr_parsing) - #163814 (Add some comments and docs related to `AllocatorNightly`)
…uwer Rollup of 5 pull requests Successful merges: - #160915 ([rustdoc] Fix how `Deref` items is handled.) - #163267 (fix `VisibleForLeakCheck` in `RegionOutlives` fast path) - #163673 (Move #[doc(inline)] and #[doc(no_inline)] validation into rustc_attr_parsing) - #163795 (When documenting without a specified `--edition`, emit a message) - #163814 (Add some comments and docs related to `AllocatorNightly`)
…uwer Rollup of 5 pull requests Successful merges: - #160915 ([rustdoc] Fix how `Deref` items is handled.) - #163267 (fix `VisibleForLeakCheck` in `RegionOutlives` fast path) - #163673 (Move #[doc(inline)] and #[doc(no_inline)] validation into rustc_attr_parsing) - #163795 (When documenting without a specified `--edition`, emit a message) - #163814 (Add some comments and docs related to `AllocatorNightly`)
…uwer Rollup of 5 pull requests Successful merges: - #160915 ([rustdoc] Fix how `Deref` items is handled.) - #163267 (fix `VisibleForLeakCheck` in `RegionOutlives` fast path) - #163673 (Move #[doc(inline)] and #[doc(no_inline)] validation into rustc_attr_parsing) - #163795 (When documenting without a specified `--edition`, emit a message) - #163814 (Add some comments and docs related to `AllocatorNightly`)
Rollup merge of #163673 - darkraider01:doc-inline-validation, r=JonathanBrouwer Move #[doc(inline)] and #[doc(no_inline)] validation into rustc_attr_parsing Part of #153101 This migrates `#[doc(inline)]` and `#[doc(no_inline)]` validation from `rustc_passes` into `DocParser` in `rustc_attr_parsing`. It checks for conflicting inlining attributes and non-use targets, and tweaks rustdoc's early abort so lowering errors don't suppress other attribute diagnostics, also added new UI tests in doc-inline-validation.rs.
View all comments
Part of #153101
This migrates
#[doc(inline)]and#[doc(no_inline)]validation fromrustc_passesintoDocParserinrustc_attr_parsing. It checks for conflicting inlining attributes and non-use targets, and tweaks rustdoc's early abort so lowering errors don't suppress other attribute diagnostics, also added new UI tests in doc-inline-validation.rs.