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

Fix false positive in missing_const_for_fn #4981

Closed
wants to merge 1 commit into from

Conversation

phansch
Copy link
Member

@phansch phansch commented Jan 2, 2020

This fixes some additional false positives with functions or methods
that return types which implement drop. Since drop can't be
const-evaluated, these cases can't be made const.

changelog: Fix false positive in [missing_const_for_fn]

Fixes #4979

@phansch phansch added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Jan 2, 2020
This fixes some additional false positives with functions or methods
that return types which implement drop. Since `drop` can't be
const-evaluated, these cases can't be made const.

Fixes rust-lang#4979
@phansch phansch force-pushed the const-fn-drop-fix branch from b3dd25d to 747e3b6 Compare January 3, 2020 05:41
@phansch phansch 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 from the author. (Use `@rustbot ready` to update this status) labels Jan 3, 2020
@flip1995
Copy link
Member

flip1995 commented Jan 4, 2020

The issue at hand talked about a second level Drop implementation. So

fn take(self) -> NotDrop {
//      ^^^^     ^^^^^^^ Doesn't implement Drop
    self.field
}

struct NotDrop {
    field: CustomDrop,
}

can you add a test for this case?

@phansch phansch added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jan 7, 2020
@phansch
Copy link
Member Author

phansch commented Jan 8, 2020

@flip1995 Hm, if take returns NotDrop (the second level), then it's fine to suggest making it const, like this:

struct CustomDrop {
    field: i32,
}

impl Drop for CustomDrop {
    fn drop(&mut self) {}
}

struct NotDrop {
    field: CustomDrop,
}

impl NotDrop {
    // This is allowed to be const because the field values are never used.
    fn take(self) -> NotDrop {
        self
    }
}

Is that what you meant?

@flip1995
Copy link
Member

flip1995 commented Jan 9, 2020

Not quiet. We have to go even further beyond: Playground

struct CustomDrop;

impl Drop for CustomDrop {
    fn drop(&mut self) {}
}

struct NotDrop {
    field: CustomDrop,
}

struct SmallTurtle { // Another layer
    field: NotDrop,
}

impl SmallTurtle {
    fn take(self) -> NotDrop {
        self.field
    }
}

That's the structure of the issue: Struct -> String -> Vec (has drop impl). I guess another test case would be to just have a struct with a String field and return this in a take function.

@phansch
Copy link
Member Author

phansch commented Jan 10, 2020

@flip1995 Yes that doesn't get detected as you expected. Hm. TyAdtDef.has_dtor only seems to check the provided type and doesn't go deeper. I never wrote a visitor before but I guess I would have to implement a sort of TyDropVisitor that calls has_drop on all nested types?

@oli-obk
Copy link
Contributor

oli-obk commented Jan 10, 2020

You may be able to just use the iterator returned by Ty::walk instead of implementing a new type visitor

@bors
Copy link
Contributor

bors commented Jan 11, 2020

☔ The latest upstream changes (presumably #5040) made this pull request unmergeable. Please resolve the merge conflicts.

@phansch
Copy link
Member Author

phansch commented Jan 23, 2020

@oli-obk Unfortunately ty.walk() does not descend into fields of structs or variants, so I think I'll have to implement that myself. However, I'm not sure if that is enough to deal with all cases?

@phansch
Copy link
Member Author

phansch commented Jan 26, 2020

I'm currently trying to use ty.needs_drop for this as it seems do exactly this. This seems fine, as it reports String (and the example case from @flip1995) as implementing Drop, but it breaks the test for String::new() and I need to figure out how to fix that.

@phansch
Copy link
Member Author

phansch commented Apr 15, 2020

Going to close this for now and revisit it at a later date 📆

@phansch phansch closed this Apr 15, 2020
@phansch phansch added the S-inactive-closed Status: Closed due to inactivity label Apr 16, 2020
@phansch phansch deleted the const-fn-drop-fix branch May 31, 2020 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive-closed Status: Closed due to inactivity S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong suggestion of const fn if member of member implements Drop
4 participants