Skip to content
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

Add filtering options to rustc_on_unimplemented #47613

Merged
merged 8 commits into from
Feb 7, 2018

Conversation

estebank
Copy link
Contributor

  • Add filtering options to rustc_on_unimplemented for local traits, filtering on Self and type arguments.
  • Add a way to provide custom notes.
  • Tweak binops text.
  • Add filter to detect wether Self is local or belongs to another crate.
  • Add filter to Iterator diagnostic for &str.

Partly addresses #44755 with a different syntax, as a first approach. Fixes #46216, fixes #37522, CC #34297, #46806.

@rust-highfive
Copy link
Collaborator

r? @Kimundi

(rust_highfive has picked a reviewer for you, use r? to override)

@estebank estebank force-pushed the rustc_on_unimplemented branch from d47b4df to ffdcd35 Compare January 20, 2018 11:11
}

fn parse_ident_common(&mut self, recover: bool) -> PResult<'a, ast::Ident> {
pub fn parse_ident_attr(&mut self) -> PResult<'a, ast::Ident> {
self.parse_ident_common(true, true)
Copy link
Contributor

@petrochenkov petrochenkov Jan 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add the logic for Self here in parse_ident_attr instead of adding one more tweak to parse_ident_common.
(See parse_path_segment_ident for an example.)

@shepmaster shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 20, 2018
@estebank estebank force-pushed the rustc_on_unimplemented branch from c42bcb7 to 9339450 Compare January 23, 2018 19:02
@estebank estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 23, 2018
@estebank
Copy link
Contributor Author

Ping.

@estebank
Copy link
Contributor Author

@nikomatsakis just as a sanity check: currently Self has only one meaning and it is not accepted anywhere else. This PR adds it as an accepted attribute argument to be able to use it in rustc_on_unimplemented filters:

#[rustc_on_unimplemented(
    on(
        Self="&str",
        label="`{Self}` is not an iterator; try calling `.chars()` or `.bytes()`"
    ),
    label="`{Self}` is not an iterator; maybe try calling `.iter()` or a similar method"
)]

Is this acceptable? Otherwise we can just keep what this PR is currently using (_Self).

@nikomatsakis
Copy link
Contributor

@estebank

currently Self has only one meaning and it is not accepted anywhere else

I .. think this is true, but I don't quite know what "anywhere else" means. Do you mean that you can't define your own type parameters named Self?

@nikomatsakis
Copy link
Contributor

If so, I believe that is true

@estebank
Copy link
Contributor Author

Self cannot be used as an identifier in the same way that let can't be used. This PR changes it so that Self is accepted as an identifier inside of attributes and keeps the current behavior everywhere else.

@estebank
Copy link
Contributor Author

Also, when adding a parser change, how long do we have to wait until we can start using that change into rustc source? For example, this PR is introducing a way for rustc_on_unimplemented to add a note. When could we start using it? Once this change hits beta? stable?

@nikomatsakis
Copy link
Contributor

Self cannot be used as an identifier in the same way that let can't be used. This PR changes it so that Self is accepted as an identifier inside of attributes and keeps the current behavior everywhere else.

Ah, I see. Hmm, that makes me a mite uncomfortable.

Also, when adding a parser change, how long do we have to wait until we can start using that change into rustc source?

You may have to wait until the next beta, I suppose, so that the code can be parsed with stage0. Sometimes a #[cfg(stage0)] flag suffices. Depends on the specifics.

@nikomatsakis
Copy link
Contributor

OK, so, I feel a bit weird about singling out Self here, and of course this is an insta-stable change. Strangely enough, I feel better about saying that we accept all keywords inside of attributes (i.e., here:

#[foo(
    let = 3,
    static = 4,
)]

than just Self. Maybe that's too crazy. =)

I sort of expect to replace on_unimplemented at some point with another mechanism, so I'm not sure I want to be driving lang design through its needs, but on the other hand this feels like something other people may want too.

Thoughts @rust-lang/lang / @petrochenkov on whether it's worrisome to allow identifiers inside of attributes like that?

@estebank
Copy link
Contributor Author

I don't think #[cfg(stage0)] would work for this.

If we decide not to start accepting kws in attributes, the workaround used here (_Self) should suffice, as no-one outside of rustc will have to be bothered by how it looks.

@estebank
Copy link
Contributor Author

estebank commented Feb 1, 2018

If we're blocked on a decision regarding allowing Self as an attribute, we could merge this PR using only _Self in the meantime.

@petrochenkov petrochenkov self-assigned this Feb 1, 2018
@nikomatsakis
Copy link
Contributor

@estebank I think i'd .. prefer that for now. This is kind of an insta-stable change I'd rather we approach carefully. That said, I think we should probably permit all attributes.

@estebank
Copy link
Contributor Author

estebank commented Feb 1, 2018

@nikomatsakis done.

pub fn parse_ident(&mut self) -> PResult<'a, ast::Ident> {
self.parse_ident_common(true)
}

pub fn parse_ident_attr(&mut self) -> PResult<'a, ast::Ident> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: now this function is unnecessary.

 - filter error on the evaluated value of `Self`
 - filter error on the evaluated value of the type arguments
 - add argument to include custom note in diagnostic
 - allow the parser to parse `Self` when processing attributes
 - add custom message to binops
@pietroalbini
Copy link
Member

@nikomatsakis ping from triage! Could you review this?

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 6, 2018

📌 Commit fd3f231 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 6, 2018
@bors
Copy link
Contributor

bors commented Feb 7, 2018

⌛ Testing commit fd3f231 with merge 51b336dc90cf0fe21beeb9184ab856985da96d79...

@bors
Copy link
Contributor

bors commented Feb 7, 2018

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 7, 2018
@kennytm
Copy link
Member

kennytm commented Feb 7, 2018

@bors retry #46903

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 7, 2018
@bors
Copy link
Contributor

bors commented Feb 7, 2018

⌛ Testing commit fd3f231 with merge a869c5e36cec50960ee1d9b482f38944c9dce01d...

@bors
Copy link
Contributor

bors commented Feb 7, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 7, 2018
@kennytm
Copy link
Member

kennytm commented Feb 7, 2018

Looks like a new kind of spurious error introduced by making trans dynamic. cc @alexcrichton @bjorn3

[01:27:12]    Compiling rustc_llvm v0.0.0 (file:///checkout/src/librustc_llvm)
[01:30:35] error: linking with `armv7-unknown-linux-gnueabihf-gcc` failed: exit code: 1
[01:30:35]   |
[01:30:35]   = note: "armv7-unknown-linux-gnueabihf-gcc" "-Wl,--as-needed" "-Wl,-z,noexecstack" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/armv7-unknown-linux-gnueabihf/lib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/armv7-unknown-linux-gnueabihf/release/deps/rustc_trans-f3b5388beaefea09.rustc_trans0.rcgu.o" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/armv7-unknown-linux-gnueabihf/release/deps/librustc_trans-f3b5388beaefea09.so" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/armv7-unknown-linux-gnueabihf/release/deps/rustc_trans-f3b5388beaefea09.crate.metadata.rcgu.o" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/armv7-unknown-linux-gnueabihf/release/deps/rustc_trans-f3b5388beaefea09.crate.allocator.rcgu.o" "-Wl,-z,relro,-z,now" "-Wl,-O1" "-nodefaultlibs" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/armv7-unknown-linux-gnueabihf/release/deps" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/release/deps" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/armv7-unknown-linux-gnueabihf/release/build/miniz-sys-2f616fcd2ada94cc/out" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/armv7-unknown-linux-gnueabihf/release/build/backtrace-sys-1eff2f55d97b6c15/out/.libs" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/armv7-unknown-linux-gnueabihf/release/build/rustc_binaryen-c4f3b3356ac4c26f/out/build/lib" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/armv7-unknown-linux-gnueabihf/release/build/rustc_binaryen-c4f3b3356ac4c26f/out" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/armv7-unknown-linux-gnueabihf/release/build/rustc_llvm-e5f4e848081985e2/out" "-L" "/checkout/obj/build/armv7-unknown-linux-gnueabihf/llvm/lib" "-L" "/x-tools/armv7-unknown-linux-gnueabihf/lib/gcc/armv7-unknown-linux-gnueabihf/4.9.3/../../../../armv7-unknown-linux-gnueabihf/lib" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/armv7-unknown-linux-gnueabihf/lib" "-Wl,-Bstatic" "-Wl,--whole-archive" "/tmp/rustc.A9KJy9ZgNIFK/libtempdir-8d8180d083453a7e.rlib" "-Wl,--no-whole-archive" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/armv7-unknown-linux-gnueabihf/release/deps" "-Wl,-Bdynamic" "-l" "rustc_trans_utils-361e76f6faaf24ea" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/armv7-unknown-linux-gnueabihf/release/deps" "-l" "rustc_platform_intrinsics-e306191fa3f3283d" "-Wl,-Bstatic" "-Wl,--whole-archive" "/tmp/rustc.A9KJy9ZgNIFK/librustc_llvm-072ac1bc6291dae6.rlib" "-Wl,--no-whole-archive" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/armv7-unknown-linux-gnueabihf/release/deps" "-Wl,-Bdynamic" "-l" "rustc_incremental-693a9318e3bc5ba0" "-Wl,-Bstatic" "-Wl,--whole-archive" "/tmp/rustc.A9KJy9ZgNIFK/librustc_binaryen-c46e349a57568498.rlib" "-Wl,--no-whole-archive" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/armv7-unknown-linux-gnueabihf/release/deps" "-Wl,-Bdynamic" "-l" "rustc_allocator-68b2261a707b01c2" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/armv7-unknown-linux-gnueabihf/release/deps" "-l" "rustc_mir-054cfe43d718be7e" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/armv7-unknown-linux-gnueabihf/lib" "-l" "rustc_const_eval-b4ca07d34b52b3cf" "-Wl,-Bstatic" "-Wl,--whole-archive" "/tmp/rustc.A9KJy9ZgNIFK/libnum_cpus-cf4970ab3a6d7bd6.rlib" "-Wl,--no-whole-archive" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/armv7-unknown-linux-gnueabihf/release/deps" "-Wl,-Bdynamic" "-l" "rustc-02479bf1e6c54756" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/armv7-unknown-linux-gnueabihf/lib" "-l" "test-3cda67f46041cedb" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/armv7-unknown-linux-gnueabihf/lib" "-l" "rustc_const_math-e9e262696674ac1a" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/armv7-unknown-linux-gnueabihf/lib" "-l" "rustc_back-3f39c8f8a35846fa" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/armv7-unknown-linux-gnueabihf/lib" "-l" "syntax-bfa1404f5e6ae027" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/armv7-unknown-linux-gnueabihf/lib" "-l" "rustc_errors-16d1e64a9f034e14" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/armv7-unknown-linux-gnueabihf/lib" "-l" "syntax_pos-16e30241347903b0" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/armv7-unknown-linux-gnueabihf/lib" "-l" "rustc_data_structures-7b29a858a5d1d542" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/armv7-unknown-linux-gnueabihf/lib" "-l" "term-e2ad26b94ed4db45" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/armv7-unknown-linux-gnueabihf/lib" "-l" "serialize-fe515b12dfcc62c4" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/armv7-unknown-linux-gnueabihf/lib" "-l" "rustc_cratesio_shim-90b98d4ff671da1f" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/armv7-unknown-linux-gnueabihf/lib" "-l" "graphviz-f1a6f6c311a8626e" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/armv7-unknown-linux-gnueabihf/lib" "-l" "fmt_macros-73c3ccf8b51ab8ac" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/armv7-unknown-linux-gnueabihf/lib" "-l" "arena-301b3ba144177cfc" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/armv7-unknown-linux-gnueabihf/lib" "-l" "std-e72d9fb337d44962" "-Wl,-Bstatic" "/tmp/rustc.A9KJy9ZgNIFK/libcompiler_builtins-8ed40bb5676824ad.rlib" "-Wl,-Bdynamic" "-l" "util" "-l" "util" "-l" "dl" "-l" "rt" "-l" "pthread" "-l" "pthread" "-l" "gcc_s" "-l" "c" "-l" "m" "-l" "rt" "-l" "pthread" "-l" "util" "-l" "util" "-shared" "-Wl,-rpath,$ORIGIN/../lib"
[01:30:35]   = note: /tmp/rustc.A9KJy9ZgNIFK/librustc_llvm-072ac1bc6291dae6.rlib(ScalarEvolution.cpp.o): In function `llvm::ScalarEvolution::isBackedgeTakenCountMaxOrZero(llvm::Loop const*)':
[01:30:35]           ScalarEvolution.cpp:(.text._ZN4llvm15ScalarEvolution29isBackedgeTakenCountMaxOrZeroEPKNS_4LoopE+0xe): relocation truncated to fit: R_ARM_THM_JUMP24 against symbol `llvm::ScalarEvolution::BackedgeTakenInfo::isMaxOrZero(llvm::ScalarEvolution*) const' defined in .text._ZNK4llvm15ScalarEvolution17BackedgeTakenInfo11isMaxOrZeroEPS0_ section in /tmp/rustc.A9KJy9ZgNIFK/librustc_llvm-072ac1bc6291dae6.rlib(ScalarEvolution.cpp.o)
[01:30:35]           collect2: error: ld returned 1 exit status

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 7, 2018
@alexcrichton
Copy link
Member

@kennytm hm that may be https://sourceware.org/bugzilla/show_bug.cgi?id=20608 and may indicate we need to update binutils on that bot...

Let's see if it happens again?

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 7, 2018
…nikomatsakis

Add filtering options to `rustc_on_unimplemented`

- Add filtering options to `rustc_on_unimplemented` for local traits, filtering on `Self` and type arguments.
- Add a way to provide custom notes.
- Tweak binops text.
- Add filter to detect wether `Self` is local or belongs to another crate.
- Add filter to `Iterator` diagnostic for `&str`.

Partly addresses rust-lang#44755 with a different syntax, as a first approach. Fixes rust-lang#46216, fixes rust-lang#37522, CC rust-lang#34297, rust-lang#46806.
bors added a commit that referenced this pull request Feb 7, 2018
Rollup of 10 pull requests

- Successful merges: #47613, #47631, #47810, #47883, #47922, #47944, #48014, #48018, #48020, #48028
- Failed merges:
@bors bors merged commit fd3f231 into rust-lang:master Feb 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[confusing error message] iterate over str #[derive] suggestion should check for local struct
10 participants