Rewrite on_unimplemented format string parser. - #139091
Conversation
This comment has been minimized.
This comment has been minimized.
|
This PR changes a file inside |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
8f91772 to
232c722
Compare
|
☔ The latest upstream changes (presumably #139555) made this pull request unmergeable. Please resolve the merge conflicts. |
|
r? @compiler-errors (or reroll) |
compiler-errors
left a comment
There was a problem hiding this comment.
ill actually look at this tomorrow, just a few actionable things until then
| --> $DIR/multiple-impls.rs:39:5 | ||
| | | ||
| LL | Index::index(&[] as &[i32], Bar(2u32)); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ on impl for Bar |
There was a problem hiding this comment.
Can you remove the rustc_on_unimplemented from impls in these test files?
There was a problem hiding this comment.
ideally we'd make it a validation error or something to put this on an impl
There was a problem hiding this comment.
so I plan on removing it in the future.
why not now? 🤔
There was a problem hiding this comment.
for a couple reasons:
- in b1bc725 I deleted its only use in std, but it's still present in the beta std. I'm unsure whether raising an error would cause problems.
- rather than adding that check to this PR I'd rather do it in a future hir attributes related PR
- I'm not intimately familiar with this code so didn't feel like raising new errors.
I'm happy to start raising errors now if you'd prefer.
There was a problem hiding this comment.
Well, I'd at least prefer for us to clean up the UI tests so they don't look like they're buggy (i.e. have annotations on impls, but don't use them).
I feel like the easiest way of doing that is just making rustc_on_unimplemented an error then clean up the new errors in the UI tests, but it's up to you I guess.
There was a problem hiding this comment.
the "now the ui tests look buggy" argument is compelling to me, i'll experiment with a hard error tomorrow
There was a problem hiding this comment.
I've deleted the tests for attribute on impls.
There was a problem hiding this comment.
ideally we'd make it a validation error or something to put this on an impl
it's not a validation error to put it anywhere, you can put it on structs, functions, etc. I would like to start erroring on all of that after migrating the attribute itself.
|
the Tidy/bootstrap labels are just because it deletes a |
|
One nit, though I didn't review this for perfect parity with the previous behavior, it looks fine to me. |
|
@rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
|
@compiler-errors ping 🙏 |
|
@bors r+ |
This PR rewrites the format string parser for
rustc_on_unimplementedanddiagnostic::on_unimplemented. I plan on moving this code (and more) into the new attribute parsing system soon and wanted to PR it separately.This PR introduces some minor differences though:
rustc_on_unimplementedon trait implementations is no longer checked/used - this is actually never used (outside of some tests) so I plan on removing it in the future.rustc_on_unimplemented, it introduces the{This}argument in favor of{ThisTraitname}(to be removed later). It'll be easier to parse.rustc_on_unimplemented,Selfcan now consistently be used as a filter, rather than just_Self. It used to not match correctly on for exampleSelf = "[{integer}]"Fixes #130627