Repository navigation
fulfill #[expect] expectations from derive generated code - #158943
Albab-Hasan wants to merge 2 commits into
Conversation
|
@Urgau hey can u pls look into this? sorry for the bother |
|
Thanks, that looks better. I will take a closer look tomorrow or Monday. You can switch to ready for review. |
|
@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.
…try> fulfill `#[expect]` expectations from derive generated code
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (c23f6fe): comparison URL. Overall result: ❌✅ regressions and improvements - 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 countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 2.6%, secondary 1.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 5.1%)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: 489.358s -> 489.445s (0.02%) |
| levels.add_command_line(); | ||
| } | ||
|
|
||
| levels.inherit_derive_expectations(owner); |
There was a problem hiding this comment.
Can you add a comment in
explaining why#[expect] is not on that list and pointing to this code?
| } | ||
| let hir::OwnerNode::Item(item) = tcx.hir_owner_node(owner) else { return }; | ||
| let hir::ItemKind::Impl(impl_) = &item.kind else { return }; | ||
| if !item.span.in_derive_expansion() { |
There was a problem hiding this comment.
Have you tested what happens with a proc-macro?
Could you add a test case with Span::call_site, Span::mixed_site and Span::def_site.
@petrochenkov I know you've worked on spans/hygiene data before. Do you know if we can trust the ExpnData will always be right?
There was a problem hiding this comment.
Of course not, procedural macros can assign any spans they want to the produced tokens.
There was a problem hiding this comment.
The general approach from #152289 seems right to me, compared to what this PR attempts to do.
Maybe in some modified form, with expects downgrated to allows or something.
Not sure why it relied on AttrIds being preserved. The mental model is that any proc macro generates tokens, and the tokens are then parsed. Built-in proc macros do optimize things by generating AST directly and skipping the parsing, but that's only an optimization, and the observable behavior should be as if the tokens were produced and parsed.
There was a problem hiding this comment.
That's what I also though, thanks for confirming it.
@Albab-Hasan unless there is a way to not rely (at all!) on the spans (and it's hygiene data), this approach seems like a dead-end.
There was a problem hiding this comment.
I'll look into it; see if anything can be done. if not I'll close this pr
There was a problem hiding this comment.
Maybe in some modified form, with
expects downgrated toallows or something.
I proposed that to T-lang, but they explicitly wants #[expect] to act as one, since a warning may only be emitted in the derived code.
Not sure why it relied on
AttrIds being preserved.
That's how the implementation of expectation works. StableLintExpectationId takes an attr_index.
There was a problem hiding this comment.
#[expect]to act as one, since a warning may only be emitted in the derived code.
Then it does require introducing some new infrastructure to preserve "item grouping", to implement this properly.
In #158980 we have a similar situation for cfg traces for derived impls used by rustdoc, but the traces can be best effort and use hacks.
|
the impl is now recognized through the gate also requires the expansion to be a builtin derive, this also holds under the tokens-then-parse model @petrochenkov described. if builtin derives went through print and reparse the impl would still be collected under the same derive the check itself is |
|
I like the idea with Both this feature and #158980 ideally need some proper design, accounting for macro calls in all positions, not just derives in item positions.
|
I guess that depends if we want user derives to behave the same as the builtin one. We currently only duplicate |
|
Still waiting for T-lang, specifically at #150553 (comment). @rustbot labels -S-waiting-on-review +S-waiting-on-t-lang |
This comment has been minimized.
This comment has been minimized.
840adfb to
a453be6
Compare
This comment has been minimized.
This comment has been minimized.
|
t-lang ruling: #150553 (comment) i rewrote it on top of the lint attribute port. it reads the adts @rustbot ready |
|
From a high level, this looks fine to me, but I know @petrochenkov had some opinions. r? petrochenkov |
|
@Urgau can you start a perf in the meantime? |
|
@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.
…try> fulfill `#[expect]` expectations from derive generated code
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (c05c3cc): 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.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.1%, secondary 3.1%)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: 484.843s -> 485.586s (0.15%) |
This comment has been minimized.
This comment has been minimized.
these record the current behaviour where an #[expect] on a derived item is not shared with the generated impls, so the lint still fires in derived code
impls generated by a builtin derive now inherit the #[expect] attributes of the derived-from item at lint level time, reusing the expectation id of the item instead of copying the attribute during expansion, so cfg re-parsing of the derive input cannot create a second expectation the lint name resolution of add_hir_inner moves into resolve_lint_check so the inheritance can reuse it without emitting diagnostics twice
a453be6 to
8c9aa73
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
View all comments
an
#[expect]on an item now covers the impls#[derive]generates from it. done at lint level building by reusing the items own expectation ids. no expansion changes at all so thecfg/cfg_attrattr id desync that forced the revert of #152289 in #153055 cant happen.context in the issue thread starting at #150553 (comment)
fixes #150553
r? @Urgau