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

NLL doesn't check that user type annotations are well-formed in unreachable code #54943

Closed
matthewjasper opened this issue Oct 9, 2018 · 8 comments

Comments

Projects
None yet
5 participants
@matthewjasper
Copy link
Contributor

commented Oct 9, 2018

#54942 checks well-formedness in the general case, but unreachable code is deleted before NLL can check it.

For example, this test still compiles with NLL enabled.

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

Discussed at NLL weekly meeting. We need to decide whether this actually is something we're going to fix or not. Thus tagging as I-needs-decision.

The test provided currently fails to compile under -Z borrowck=migrate and thus we believe this problem will be flagged under the 2018 edition.

That gives us plenty of time to decide how we want to handle this in the long term.

Nonetheless, putting on the Release milestone so that we do actually make the aforementioned decision about whether to fix this.

@pnkfelix pnkfelix added this to the Rust 2018 Release milestone Oct 16, 2018

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

I was thinking more about this. I think perhaps the way to fix this is as follows:

  • Modify the MIR to put "user-provided type annotations" into a separate IndexVec that is part of the Mir struct, so that they are not embedded into random statements that will be removed by dead-code. Modify the statements and things to just keep an index into that vec. They should perhaps have a span though associated with them.
  • Modify the NLL type-check to:
    • on entry, instantiate each of the user-given type annotations once with fresh type variables
    • also add Wf obligations for each of the resulting types (this subsumes the existing code that adds such obligations) with location Locations:All
    • for the statements that assert types, we just index into the "instantiated" vector to pull out the relevant type
@gnzlbg

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2018

See also #55344 .

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

triage: Either this is something that we're planning to implement and backport to the beta channel... or its something that is not that high priority.

Given that this is an NLL-sound issue, I'm going to assume that it is indeed high priority and belongs on the Release milestone.

Thus, tagging as P-high

@pnkfelix pnkfelix added the P-high label Nov 6, 2018

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

discussed at NLL meeting. @matthewjasper pointed out to me that this bug does not apply to migration mode, only to #![feature(nll)].

So, I am removing this from the Release milestone.

@pnkfelix pnkfelix removed this from the Rust 2018 Release milestone Nov 6, 2018

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

Oh; looking over this now, I realize that the whole reason this was on the Release milestone was to resolve the I-needs-decision in a timely fashion. (And I was the one who even documented this, 21 days ago. 🤥 )

Putting back on the Release milestone.

@pnkfelix pnkfelix added this to the Rust 2018 Release milestone Nov 6, 2018

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

assigning to self for investigation of the issues that should drive the decision here (and perhaps the actual implementation).

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2018

I'm going to remove this from the release milestone. The current behavior is not even necessarily a bug, and it's definitely obscure and not something I expect a lot of people to run into. Moreover, the changes to fix those look fairly invasive, and I'm not eager to backport them -- I could easily imagine surprise ICEs resulting from such changes.

@nikomatsakis nikomatsakis removed this from the Rust 2018 Release milestone Nov 16, 2018

bors added a commit that referenced this issue Dec 21, 2018

Auto merge of #55937 - davidtwco:issue-54943, r=pnkfelix
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

bors added a commit that referenced this issue Jan 1, 2019

Auto merge of #55937 - davidtwco:issue-54943, r=pnkfelix
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

@davidtwco davidtwco assigned matthewjasper and unassigned davidtwco Jan 16, 2019

bors added a commit that referenced this issue Jan 25, 2019

Auto merge of #57714 - matthewjasper:wellformed-unreachable, r=pnkfelix
[NLL] Clean up handling of type annotations

* Renames (Canonical)?UserTypeAnnotation -> (Canonical)?UserType so that the name CanonicalUserTypeAnnotation is free.
* Keep the inferred type associated to user type annotations in the MIR, so that it can be compared against the annotated type, even when the annotated expression gets removed from the MIR. (#54943)
* Use the inferred type to allow infallible handling of user type projections (#57531)
* Uses revisions for the tests in #56993
* Check the types of `Unevaluated` constants with no annotations (#46702)
* Some drive-by cleanup

Closes #46702
Closes #54943
Closes #57531
Closes #57731
cc #56993 leaving this open to track the underlying issue: we are not running tests with full NLL enabled on CI at the moment

r? @nikomatsakis

@bors bors closed this in #57714 Jan 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.