Rustup to rust-lang/rust#64874 #4628

flip1995 · 2019-10-04T13:11:45Z

TODO:

replace rvalue_promotable_map in [1]
~~fix [2] according to this comment Simplify ExprUseVisitor rust#64874 (comment) this should be merged with consume, but I didn't figure out how to merge them, yet.~~
~~fix [3]; What to do with LoanCause?~~

[2]+[3] probably have to be resolved by a rewrite of the lint. #4628 (comment)

[1]

rust-clippy/clippy_lints/src/methods/mod.rs

Lines 1292 to 1299 in 54bf4ff

    
           let promotable = self 
        
               .cx 
        
               .tcx 
        
               .rvalue_promotable_map(owner_def) 
        
               .contains(&expr.hir_id.local_id); 
        
           if !promotable { 
        
               self.found |= true; 
        
           }

[2]

rust-clippy/clippy_lints/src/escape.rs

Line 126 in 54bf4ff

    
           fn consume_pat(&mut self, consume_pat: &Pat, cmt: &cmt_<'tcx>, _: ConsumeMode) {

[3]

rust-clippy/clippy_lints/src/escape.rs

Lines 166 to 176 in 54bf4ff

    
           fn borrow( 
        
               &mut self, 
        
               _: HirId, 
        
               _: Span, 
        
               cmt: &cmt_<'tcx>, 
        
               _: ty::Region<'_>, 
        
               _: ty::BorrowKind, 
        
               loan_cause: LoanCause, 
        
           ) { 
        
               if let Categorization::Local(lid) = cmt.cat { 
        
                   match loan_cause {

I could need some help with [1]. The purpose of this is to "don't lint for constant values". cc @matthewjasper

For now I see what I can do with [2].

changelog: Temporary break boxed_local lint.

Episode 1 - The simple cases

This also accidentally improved the spans of the suggestions

flip1995 · 2019-10-04T13:49:09Z

clippy_lints/src/needless_pass_by_value.rs

        let cmt = unwrap_downcast_or_interior(cmt);

        if let mc::Categorization::Local(vid) = cmt.cat {
            self.moved_vars.insert(vid);
        }
    }
-
-    fn non_moving_pat(&mut self, matched_pat: &Pat, cmt: &mc::cmt_<'tcx>) {


Is removing this completely ok? It didn't break anything in the tests 🤔 cc @sinkuu since you added this function in 627d24c

Nothing is inserted to spans_need_deref after this change, so the suggestions to add * should be broken, but idk.

Anyway, Rust has gained default binding mode and this suggestion became useless. I think it's OK to remove it.

flip1995 · 2019-10-04T17:55:49Z

Update: I won't get to this until tomorrow. It would be great if someone could take over :)

matthewjasper · 2019-10-04T22:05:02Z

[1] Change is_ctor_function to check for promotable functions as well (and give it a different name). It would look something like

// ...
match cx.tables.qpath_res(qp, fun.hir_id) {
    def::Res::Def(DefKind::Variant, ..) | Res::Def(DefKind::Ctor(..), _) => true,
    // Maybe consider `is_const_fn`, but there's less guarantee of cheapness there.
    def::Ref::Def(DefKind::Fn, def_id) => cx.tcx.is_promotable_const_fn(def_id),
    _ => false,
}

[2] and [3] Looking into this lint some more, it's hopelessly broken and needs a rewrite. That said, to roughly keep the current behavior:

[2]:

Move the if is_argument part into consume, using cmt.hir_id instead of consume_pat.hir_id.
Remove the if let Categorization::Rvalue(..) = ... block, it doesn't appear to work.
Move the if let Categorization::Local(lid) = ... block into consume adding a check that checking that map.find(cmt.hir_id) is Some(Node::Binding(_))

[3]: Unconditionally call self.set.remove(&lid); - this slightly changes the behavior of this lint around closures.

FIXME: This doesn't work and probably needs a rewrite of the lint See rust-lang#4628 (comment)

flip1995 · 2019-10-05T10:26:48Z

Thanks a lot for the hints @matthewjasper!

Oh nevermind...

u32::max_value() (and so on) is a DefKind::Method, not DefKind::Fn

[1] That doesn't work, because the PathSegment of a const fn call like u32::max_value() always has res: Some(Err), e.g.

let _ = 1u32.checked_add(1).unwrap_or(u32::max_value());

dbg!(qpath):

[clippy_lints/src/utils/mod.rs:809] qp = TypeRelative(
    type(u32),
    PathSegment {
        ident: max_value#0,
        hir_id: Some(
            HirId {
                owner: DefIndex(17),
                local_id: 43,
            },
        ),
        res: Some(
            Err,
        ),
        args: None,
        infer_args: true,
    },
)

I'm still curious about this though:

What does Res::Err mean? That the resolution errored or that no resolution is known at this point? (Does Res even stand for Resolution?) Sorry if that's a stupid question, but def::Res is completely undocumented and I couldn't find anything in the rustc-guide.

it's hopelessly broken and needs a rewrite

That's what I was afraid of. I propably won't have the time of doing a rewrite, so I'd suggest to just adapt the tests, since this only introduces FNs. We then can get a working Clippy faster and work on this later. cc @rust-lang/clippy

[2]+[3] I did this is in b7d4735, but the result is, that all tests fail for this lint

flip1995 · 2019-10-05T11:23:37Z

On another note: I think it's time to Nuke the clippy plugin interface

warning: use of deprecated attribute `plugin_registrar`: compiler plugins are deprecated. See https://github.com/rust-lang/rust/issues/29597
  --> src/lib.rs:12:1
   |
12 | #[plugin_registrar]
   | ^^^^^^^^^^^^^^^^^^^ help: remove this attribute
   |
   = note: `#[warn(deprecated)]` on by default

https://travis-ci.com/rust-lang/rust-clippy/jobs/242398220#L890-L896

flip1995 · 2019-10-05T11:44:28Z

As suggested in #4616 (comment)

@bors treeclosed=10 p=10

matthewjasper · 2019-10-06T09:49:17Z

What does Res::Err mean? That the resolution errored or that no resolution is known at this point? (Does Res even stand for Resolution?) Sorry if that's a stupid question, but def::Res is completely undocumented and I couldn't find anything in the rustc-guide.

Res::Err usually means that name resolution errored, but it's also used here because the name won't be resolved until type checking is done.

[2]+[3] I did this is in b7d4735, but the result is, that all tests fail for this lint

I've had a look at this, moving the is_argument check to mutate fixes this (only the error that I expected to go is now gone).

flip1995 · 2019-10-07T07:47:24Z

tests/ui/escape_analysis.stderr

@@ -12,11 +12,5 @@ error: local variable doesn't need to be boxed here
 LL | pub fn new(_needs_name: Box<PeekableSeekable<&()>>) -> () {}
   |            ^^^^^^^^^^^

-error: local variable doesn't need to be boxed here


This is the only remaining FN introduced in this PR now. So ping @rust-lang/clippy for review.

file an issue for it and refer to it in the test file and we can land tis

phansch · 2019-10-08T05:09:46Z

@bors r+ p=10

bors · 2019-10-08T05:09:48Z

📌 Commit 3d39379 has been approved by phansch

bors · 2019-10-08T05:09:57Z

⌛ Testing commit 3d39379 with merge df80193...

@matthewjasper

Rustup to rust-lang/rust#64874 TODO: - [x] replace `rvalue_promotable_map` in [1] - [ ] ~~fix [2] according to this comment rust-lang/rust#64874 (comment) this should be merged with `consume`, but I didn't figure out how to merge them, yet.~~ - [ ] ~~fix [3]; What to do with `LoanCause`?~~ [2]+[3] probably have to be resolved by a rewrite of the lint. #4628 (comment) [1] https://github.com/rust-lang/rust-clippy/blob/54bf4ffd626970e831bb80c037f804a3b3450835/clippy_lints/src/methods/mod.rs#L1292-L1299 [2] https://github.com/rust-lang/rust-clippy/blob/54bf4ffd626970e831bb80c037f804a3b3450835/clippy_lints/src/escape.rs#L126 [3] https://github.com/rust-lang/rust-clippy/blob/54bf4ffd626970e831bb80c037f804a3b3450835/clippy_lints/src/escape.rs#L166-L176 I could need some help with [1]. The purpose of this is to "don't lint for constant values". cc @matthewjasper For now I see what I can do with [2]. changelog: Temporary break `boxed_local` lint.

bors · 2019-10-08T05:46:40Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: phansch
Pushing df80193 to master...

phansch · 2019-10-08T05:46:53Z

@bors treeclosed-

flip1995 force-pushed the rustup branch 2 times, most recently from ab95b45 to 65a52ba Compare October 4, 2019 13:33

flip1995 added 2 commits October 4, 2019 15:39

Rustup to rust-lang/rust#64874

b46f5b4

Episode 1 - The simple cases

Fix needless_pass_by_value

c420b07

This also accidentally improved the spans of the suggestions

flip1995 force-pushed the rustup branch from 65a52ba to c420b07 Compare October 4, 2019 13:40

flip1995 commented Oct 4, 2019

View reviewed changes

flip1995 added the E-help-wanted Call for participation: Help is requested to fix this issue. label Oct 4, 2019

This was referenced Oct 4, 2019

Fix license in Cargo.toml files #4616

Merged

Allow everyone to assign labels with rustbot #4595

Merged

This comment has been minimized.

Sign in to view

Merge consume and consume_pat in escape analysis

b7d4735

FIXME: This doesn't work and probably needs a rewrite of the lint See rust-lang#4628 (comment)

flip1995 added 2 commits October 5, 2019 12:38

Get rid of rvalue_promotable_map method call

3b23092

Run util/dev fmt

3aa531c

flip1995 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed E-help-wanted Call for participation: Help is requested to fix this issue. labels Oct 5, 2019

flip1995 changed the title ~~WIP: Rustup to rust-lang/rust#64874~~ Rustup to rust-lang/rust#64874 Oct 5, 2019

flip1995 mentioned this pull request Oct 5, 2019

ice on glacier 28189.rs #4631

Closed

Move is_argument check into mutate

3d39379

flip1995 commented Oct 7, 2019

View reviewed changes

Manishearth approved these changes Oct 7, 2019

View reviewed changes

bors merged commit 3d39379 into rust-lang:master Oct 8, 2019

flip1995 deleted the rustup branch October 8, 2019 06:50

flip1995 mentioned this pull request Oct 8, 2019

FN in boxed_local lint #4642

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rustup to rust-lang/rust#64874 #4628

Rustup to rust-lang/rust#64874 #4628

flip1995 commented Oct 4, 2019 •

edited

Loading

flip1995 Oct 4, 2019 •

edited

Loading

sinkuu Oct 4, 2019

This comment has been minimized.

flip1995 commented Oct 4, 2019

matthewjasper commented Oct 4, 2019

flip1995 commented Oct 5, 2019 •

edited

Loading

flip1995 commented Oct 5, 2019

flip1995 commented Oct 5, 2019

matthewjasper commented Oct 6, 2019

flip1995 Oct 7, 2019

Manishearth Oct 7, 2019

phansch commented Oct 8, 2019

bors commented Oct 8, 2019

bors commented Oct 8, 2019

bors commented Oct 8, 2019

phansch commented Oct 8, 2019 •

edited

Loading

	let promotable = self
	.cx
	.tcx
	.rvalue_promotable_map(owner_def)
	.contains(&expr.hir_id.local_id);
	if !promotable {
	self.found \|= true;
	}

	fn borrow(
	&mut self,
	_: HirId,
	_: Span,
	cmt: &cmt_<'tcx>,
	_: ty::Region<'_>,
	_: ty::BorrowKind,
	loan_cause: LoanCause,
	) {
	if let Categorization::Local(lid) = cmt.cat {
	match loan_cause {

Rustup to rust-lang/rust#64874 #4628

Rustup to rust-lang/rust#64874 #4628

Conversation

flip1995 commented Oct 4, 2019 • edited Loading

flip1995 Oct 4, 2019 • edited Loading

Choose a reason for hiding this comment

sinkuu Oct 4, 2019

Choose a reason for hiding this comment

This comment has been minimized.

flip1995 commented Oct 4, 2019

matthewjasper commented Oct 4, 2019

flip1995 commented Oct 5, 2019 • edited Loading

flip1995 commented Oct 5, 2019

flip1995 commented Oct 5, 2019

matthewjasper commented Oct 6, 2019

flip1995 Oct 7, 2019

Choose a reason for hiding this comment

Manishearth Oct 7, 2019

Choose a reason for hiding this comment

phansch commented Oct 8, 2019

bors commented Oct 8, 2019

bors commented Oct 8, 2019

bors commented Oct 8, 2019

phansch commented Oct 8, 2019 • edited Loading

flip1995 commented Oct 4, 2019 •

edited

Loading

flip1995 Oct 4, 2019 •

edited

Loading

flip1995 commented Oct 5, 2019 •

edited

Loading

phansch commented Oct 8, 2019 •

edited

Loading