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

handle user type annotations in NLL for ref bindings #55401

Closed
pnkfelix opened this issue Oct 26, 2018 · 5 comments
Closed

handle user type annotations in NLL for ref bindings #55401

pnkfelix opened this issue Oct 26, 2018 · 5 comments
Assignees
Labels
A-NLL Area: Non-lexical lifetimes (NLL) NLL-sound Working towards the "invalid code does not compile" goal P-medium Medium priority

Comments

@pnkfelix
Copy link
Member

spun off from #47184 and #54570 :

Most of the necessary support for checking lifetime constraints introduced via complex patterns has been added. But one exceptional case which has not been properly dealt with is ref bindings.

If you look here the code explicitly says "ignore pattern_user_ty for now."

BindingMode::ByRef(..) => {
// If this is a `ref` binding (e.g., `let ref
// x: T = ..`), then the type of `x` is not
// `T` but rather `&T`, so ignore
// `pattern_user_ty` for now.
//
// FIXME(#47184): extract or handle `pattern_user_ty` somehow
pattern_user_ty = None;
}

(I fixed a lot of instances of FIXME(#47184) in PR #55274, but I punted on this one.)

@nikomatsakis
Copy link
Contributor

@pnkfelix do you have an example of a program that fails to error as a result of this bug?

@pnkfelix
Copy link
Member Author

I think this should error, but does not under NLL today:

fn static_to_a_to_static_through_ref_in_tuple<'a>(x: &'a u32) -> &'static u32 {
    let (ref y, _z): (&'a u32, u32) = (&22, 44);
    *y //~ ERROR
}

@pnkfelix
Copy link
Member Author

(this may not be that hard to fix. I punted because the other cases seemed way more important to get fixed ASAP, but its possible that a similar pattern can work for this too.)

@pnkfelix
Copy link
Member Author

I don't think any fix for this is worth the effort of backporting, though... its a really subtle case.

@pnkfelix pnkfelix added the P-medium Medium priority label Oct 30, 2018
@davidtwco
Copy link
Member

davidtwco commented Nov 24, 2018

Update: Got confused by migration mode, this isn't fixed.

The test case in this issue now errors correctly under NLL, seems to have been changed by #55274, but that PR description says:

Oh, one more thing: This doesn't attempt to handle ref x (in terms of ensuring that any necessary types are ascribed to x in that scenario as well). We should open an issue to investigate supporting that as well. But I didn't want to block this PR on that future work.

@davidtwco davidtwco self-assigned this Nov 25, 2018
@matthewjasper matthewjasper added NLL-sound Working towards the "invalid code does not compile" goal and removed NLL-deferred labels Dec 8, 2018
bors added a commit that referenced this issue Jan 1, 2019
NLL: User type annotations refactor, associated constant patterns and ref bindings.

Fixes #55511 and Fixes #55401. Contributes to #54943.

This PR performs a large refactoring on user type annotations, checks user type annotations for associated constants in patterns and that user type annotations for `ref` bindings are respected.

r? @nikomatsakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non-lexical lifetimes (NLL) NLL-sound Working towards the "invalid code does not compile" goal P-medium Medium priority
Projects
None yet
Development

No branches or pull requests

4 participants