add test to reproduce #137687 and fix it by converting #[crate_name] to a new-style attribute parser
#137729
Conversation
rustbot
added
A-attributes
This comment has been minimized.
This comment has been minimized.
jdonszelmann
force-pushed
the
fix-137687
branch
from
04bafec to
3898182
Compare
This comment has been minimized.
This comment has been minimized.
jdonszelmann
force-pushed
the
fix-137687
branch
from
3898182 to
81fc9d1
Compare
jdonszelmann
force-pushed
the
fix-137687
branch
2 times, most recently
from
4e90cee to
21838d5
Compare
jdonszelmann
force-pushed
the
fix-137687
branch
from
21838d5 to
d1ee9c4
Compare
| name: Symbol, | ||
| name_span: Span, |
There was a problem hiding this comment.
This could theoretically use Spanned but it doesn't necessarily lead to more readable code (and I don't remember if it's compatible with translatable diagnostics).
| name: Symbol, | |
| name_span: Span, | |
| name: Symbol, | |
| name_span: Span, |
| pub(crate) struct CratenameParser; | ||
|
|
||
| impl SingleAttributeParser for CratenameParser { | ||
| const PATH: &'static [Symbol] = &[sym::crate_name]; |
There was a problem hiding this comment.
FYI: You generally no longer need to explicitly state 'static in the ty of assoc consts.
|
|
||
| fn on_duplicate(cx: &AcceptContext<'_>, first_span: Span) { | ||
| // FIXME(jdonszelmann): better duplicate reporting (WIP) | ||
| cx.emit_err(UnusedMultiple { |
There was a problem hiding this comment.
Duplicates are warnings on master, right? So this is a breaking change?
|
@fmease and I decided to delay this PR for a bit until more diagnostic infra for attrs lands (hopefully next week) to make the duplicate error a warning to be consistent with current master. |
fmease
added
S-blocked
|
r=fmease,bjorn3 once that PR lands and you can downgrade the error to a lint warning again. @bors delegate+ |
|
✌️ @jdonszelmann, you can now approve this pull request! If @fmease told you to " |
|
☔ The latest upstream changes (presumably #138416) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Hi @jdonszelmann and @fmease, #137687 has reached beta so it would be great if we could resolve that via a beta-backport of this PR. Do you think it's possible to resolve the review feedback and merge this PR in the next week or so? I can probably pitch in to help if necessary 🙂 |
|
Hi there. We wanted to delay this until attribute diagnostics. Something that should not be a lot of work but I have not worked as much on recently as I maybe should. I can't to this tomorrow, but likely next week. The pr has been reviewed but there was a bit of an issue around incremental, which I now know how to solve but just haven't yet. Is that a reasonable timeframe? I am pretty busy with organising rust week atm, but knowing the solution I guess the only important thing is that it gets quick reviews? Let me know how that sounds |
|
Just to clarify, will this PR then depend on the attribute diagnostics work? (Is that #138164?) Generally, for backports, we try to keep the backport as small as possible so if this PR can be made to work independently of that PR (possibly even with a degraded diagnostics experience), that would be highly preferred. |
|
No. We decided to delay this because it's relatively low prio and any nice and permanent solution for this depends on that. I'll make a temp fix that at least doesn't ice separately then, that sounds better for now. I don't mind doing it, I know the problem now and then I also know how to fix it permanently later. You'll hear from me |
r? @fmease
Closes #137687