refactor the `BorrowckErrors` trait to take `fn(self)` #48902

csmoe · 2018-03-10T04:33:23Z

rust-highfive · 2018-03-10T04:33:28Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

petrochenkov · 2018-03-10T21:38:33Z

src/librustc_mir/util/borrowck_errors.rs

-pub trait BorrowckErrors {
-    fn struct_span_err_with_code<'a, S: Into<MultiSpan>>(&'a self,
+pub trait BorrowckErrors<'cx> {
+    fn struct_span_err_with_code<S: Into<MultiSpan>>(self,
                                                         sp: S,
                                                         msg: &str,


Formatting is broken here in in few other places.

petrochenkov · 2018-03-10T21:38:51Z

LGTM, r=me after fixing formatting

csmoe · 2018-03-11T01:05:12Z

r @petrochenkov
formatting fixed.

petrochenkov · 2018-03-11T23:13:00Z

@bors r+

bors · 2018-03-11T23:13:01Z

📌 Commit cb5ac97 has been approved by petrochenkov

…lf, r=petrochenkov refactor the `BorrowckErrors` trait to take `fn(self)` Fixes rust-lang#48783

nikomatsakis · 2018-03-14T20:16:21Z

src/librustc_mir/util/borrowck_errors.rs

+                                           sp: S,
+                                           msg: &str)
+                                           -> DiagnosticBuilder<'cx>
+        where Self: Sized + Copy


Wait -- this where clause should not be needed, right? (I would have probably moved it into the trait, in any case, but it shouldn't be needed in the impls...)

nikomatsakis · 2018-03-15T16:23:15Z

src/librustc_mir/util/borrowck_errors.rs

+                              diag: DiagnosticBuilder<'cx>,
+                              o: Origin)
+                              -> DiagnosticBuilder<'cx>
+        where Self: Sized + Copy;


Nit: I would move these into the trait definition, as the trait is meant to be implemented for references.

nikomatsakis · 2018-03-15T16:23:28Z

src/librustc_mir/util/borrowck_errors.rs

@@ -284,22 +298,25 @@ pub trait BorrowckErrors {
        self.cancel_if_wrong_origin(err, o)
    }

-    fn cannot_assign(&self, span: Span, desc: &str, o: Origin) -> DiagnosticBuilder
+    fn cannot_assign(self, span: Span, desc: &str, o: Origin) -> DiagnosticBuilder<'cx>
+        where Self: Sized + Copy


...then you could remove them from all these places

it seems the where clause cannot be removed here:

error[E0277]: the trait bound `Self: std::marker::Sized` is not satisfied --> librustc_mir/util/borrowck_errors.rs:287:22 | 287 | fn cannot_assign(self, span: Span, desc: &str, o: Origin) -> DiagnosticBuilder<'cx> | ^^^^ `Self` does not have a constant size known at compile-time | = help: the trait `std::marker::Sized` is not implemented for `Self` = help: consider adding a `where Self: std::marker::Sized` bound = note: all local variables must have a statically known size

@nikomatsakis
the compiler complained that the self in trait should be Sized + Copy, and I found that TyCtxt already #[derive(Copy, Clone)], so I rewrote pub trait BorrowckError<'tcx>: Sized + Copy {} instead of appending where clause in every method. Is this make sense?

Yep! That's what I was proposing, in fact.

nikomatsakis

Thanks @csmoe -- left one more nit :)

nikomatsakis · 2018-03-19T20:12:16Z

@bors r+ rollup

bors · 2018-03-19T20:12:17Z

📌 Commit 5511624 has been approved by nikomatsakis

…lf, r=nikomatsakis refactor the `BorrowckErrors` trait to take `fn(self)` Fixes rust-lang#48783

Rollup of 17 pull requests - Successful merges: #46518, #48810, #48834, #48902, #49004, #49092, #49096, #49099, #49104, #49125, #49139, #49152, #49157, #49161, #49166, #49176, #49184 - Failed merges:

rust-highfive assigned petrochenkov Mar 10, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 10, 2018

petrochenkov reviewed Mar 10, 2018

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 10, 2018

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 11, 2018

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Mar 12, 2018

Rollup merge of rust-lang#48902 - csmoe:refactor_BorrowckErrors_fn_se…

03a4f9d

…lf, r=petrochenkov refactor the `BorrowckErrors` trait to take `fn(self)` Fixes rust-lang#48783

Mark-Simulacrum mentioned this pull request Mar 12, 2018

Rollup of 8 pull requests #48947

Closed

nikomatsakis reviewed Mar 14, 2018

View reviewed changes

nikomatsakis reviewed Mar 15, 2018

View reviewed changes

nikomatsakis requested changes Mar 15, 2018

View reviewed changes

petrochenkov assigned nikomatsakis and unassigned petrochenkov Mar 16, 2018

csmoe added 2 commits March 16, 2018 14:52

change &self to self and fix lifetime annotations

0b111e6

fix formatting

c62d9eb

csmoe force-pushed the refactor_BorrowckErrors_fn_self branch 2 times, most recently from 813c57d to 4d24236 Compare March 18, 2018 11:16

remove unneeded where clause

5511624

csmoe force-pushed the refactor_BorrowckErrors_fn_self branch from 4d24236 to 5511624 Compare March 18, 2018 12:18

kennytm added a commit to kennytm/rust that referenced this pull request Mar 19, 2018

Rollup merge of rust-lang#48902 - csmoe:refactor_BorrowckErrors_fn_se…

7aa6651

…lf, r=nikomatsakis refactor the `BorrowckErrors` trait to take `fn(self)` Fixes rust-lang#48783

kennytm mentioned this pull request Mar 19, 2018

Rollup of 17 pull requests #49190

Merged

bors merged commit 5511624 into rust-lang:master Mar 20, 2018

csmoe deleted the refactor_BorrowckErrors_fn_self branch July 13, 2018 08:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor the `BorrowckErrors` trait to take `fn(self)` #48902

refactor the `BorrowckErrors` trait to take `fn(self)` #48902

csmoe commented Mar 10, 2018

rust-highfive commented Mar 10, 2018

petrochenkov Mar 10, 2018

petrochenkov commented Mar 10, 2018

csmoe commented Mar 11, 2018

petrochenkov commented Mar 11, 2018

bors commented Mar 11, 2018

nikomatsakis Mar 14, 2018

nikomatsakis Mar 15, 2018

nikomatsakis Mar 15, 2018

csmoe Mar 17, 2018

csmoe Mar 18, 2018 •

edited

Loading

nikomatsakis Mar 19, 2018 •

edited

Loading

nikomatsakis left a comment

nikomatsakis commented Mar 19, 2018

bors commented Mar 19, 2018

refactor the BorrowckErrors trait to take fn(self) #48902

refactor the BorrowckErrors trait to take fn(self) #48902

Conversation

csmoe commented Mar 10, 2018

rust-highfive commented Mar 10, 2018

petrochenkov Mar 10, 2018

Choose a reason for hiding this comment

petrochenkov commented Mar 10, 2018

csmoe commented Mar 11, 2018

petrochenkov commented Mar 11, 2018

bors commented Mar 11, 2018

nikomatsakis Mar 14, 2018

Choose a reason for hiding this comment

nikomatsakis Mar 15, 2018

Choose a reason for hiding this comment

nikomatsakis Mar 15, 2018

Choose a reason for hiding this comment

csmoe Mar 17, 2018

Choose a reason for hiding this comment

csmoe Mar 18, 2018 • edited Loading

Choose a reason for hiding this comment

nikomatsakis Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis commented Mar 19, 2018

bors commented Mar 19, 2018

refactor the `BorrowckErrors` trait to take `fn(self)` #48902

refactor the `BorrowckErrors` trait to take `fn(self)` #48902

csmoe Mar 18, 2018 •

edited

Loading

nikomatsakis Mar 19, 2018 •

edited

Loading