Skip to content

Borrowchecker regression in 1.26 #49945

Description

@emilyalbini

Activity

  1. added
    T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.
    C-bugCategory: This is a bug.
    on Apr 13, 2018
  2. added this to the 1.26 milestone on Apr 13, 2018
  3. emilyalbini commented on Apr 16, 2018

    @emilyalbini
    MemberAuthor
  4. added a commit that references this issue on Apr 17, 2018
  5. self-assigned this
    on Apr 19, 2018
  6. pnkfelix commented on Apr 19, 2018

    @pnkfelix
    Contributor

    visiting for triage.

    @pietroalbini are you in a position to narrow this into a stand-alone test case?

  7. pnkfelix commented on Apr 19, 2018

    @pnkfelix
    Contributor

    triage : P-high

  8. emilyalbini commented on Apr 19, 2018

    @emilyalbini
    MemberAuthor

    are you in a position to narrow this into a stand-alone test case?

    That was just from crater, I can try bisecting in a few hours though.

  9. emilyalbini commented on Apr 19, 2018

    @emilyalbini
    MemberAuthor

    @pnkfelix the bisect run points inside of the rollup #49337, and #49299 (edit: wrong copy/paste!) is the one that makes more sense (since the issue affects closures). I'll try investigating more later today.

  10. emilyalbini commented on Apr 24, 2018

    @emilyalbini
    MemberAuthor

    Didn't have time to investigate this in the past days, sorry! cc @nikomatsakis

  11. self-assigned this
    on Apr 26, 2018
  12. nikomatsakis commented on Apr 26, 2018

    @nikomatsakis
    Contributor

    OK, I see what is happening. Fascinating; I did not anticipate this side-effect of making closures implement Copy. I think the problem is this:

    • Closures were never Copy before.
    • This code creates a closure skip_headers
    • This closure is then used from another closure we'll call skip; skip escapes from the fn.
      • In the olden days, because the closure was not copy, it was implicitly moved into skip
      • But now, the closure is copy, and therefore it is taken by shared reference and copied out
        • hence skip must be declared as move

    I guess this is "working as expected", but it's a surprise interaction I had not considered in advance. (More generally, it seems to suggest that making any struct Copy could break closures down the line.)

    cc @rust-lang/lang

  13. nikomatsakis commented on Apr 26, 2018

    @nikomatsakis
    Contributor

    Still, I don't think it's worth reverting the change, and we are certainly not going to change this aspect of closure upvar inference; if we did, then a TON of things would stop working (basically any FnOnce that moves but is not declared move).

  14. nikomatsakis commented on Apr 26, 2018

    @nikomatsakis
    Contributor

    Idea, discussed in gitter:

    In the edition, if closures are not declared as move but reference non-zero upvars, then we could give them a phantom lifetime to prevent them from escaping the enclosing function. This would prevent the "semver-fail" that we observe here, where adding Copy to a type breaks consumers.

  15. emilyalbini commented on May 1, 2018

    @emilyalbini
    MemberAuthor

    Ping @nikomatsakis! We're approaching the release of 1.26, what should we do with this regression?

  16. nikomatsakis commented on May 2, 2018

    @nikomatsakis
    Contributor

    @pietroalbini I believe we are going to categorize this as "won't fix". The crate in question has already worked around it, in any case.

  17. emilyalbini commented on May 2, 2018

    @emilyalbini
    MemberAuthor

    @nikomatsakis ok, closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

A-borrow-checkerArea: The borrow checkerC-bugCategory: This is a bug.P-highHigh priorityT-compilerRelevant to the compiler team, which will review and decide on the PR/issue.regression-from-stable-to-betaPerformance or correctness regression from stable to beta.

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions