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: User type annotations refactor, associated constant patterns and ref bindings. #55937

Merged
merged 11 commits into from
Jan 1, 2019

Conversation

davidtwco
Copy link
Member

@davidtwco davidtwco commented Nov 13, 2018

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 13, 2018
@davidtwco

This comment has been minimized.

@rust-highfive

This comment has been minimized.

src/librustc/mir/mod.rs Outdated Show resolved Hide resolved
src/librustc/mir/visit.rs Outdated Show resolved Hide resolved
src/librustc_mir/borrow_check/nll/type_check/mod.rs Outdated Show resolved Hide resolved
@davidtwco

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@davidtwco davidtwco force-pushed the issue-54943 branch 2 times, most recently from 052aedc to c3a9f4d Compare November 16, 2018 22:41
@rust-highfive

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Left one nit.

r=me once tests passed and comment is added

src/librustc_mir/borrow_check/nll/type_check/mod.rs Outdated Show resolved Hide resolved
@nikomatsakis nikomatsakis changed the title WIP: NLL doesn't check that user type annotations are well-formed in unreachable code NLL doesn't check that user type annotations are well-formed in unreachable code Nov 20, 2018
@davidtwco davidtwco force-pushed the issue-54943 branch 3 times, most recently from ef8f048 to 5069d62 Compare November 22, 2018 23:11
@davidtwco
Copy link
Member Author

Pushed the first version of this that does what it says on the tin. Will need a fresh review as a lot has changed.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@davidtwco davidtwco changed the title NLL doesn't check that user type annotations are well-formed in unreachable code NLL: User type annotations in unreachable code and associated constant patterns. Nov 24, 2018
@davidtwco
Copy link
Member Author

davidtwco commented Nov 24, 2018

Added a fix for #55511 to this PR. Still seeing an ICE on three tests for the original fix, worked out that is due to this addition:

https://github.com/rust-lang/rust/blob/bde1f0d98bd1927baf5f53bd5c0d54d1382f1c45/src/librustc_traits/type_op.rs#L151-L154

But without that addition, the test change in src/test/ui/regions/regions-free-region-ordering-caller1.rs doesn't happen. Still working on it.

@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@davidtwco davidtwco changed the title NLL: User type annotations in unreachable code and associated constant patterns. NLL: User type annotations in unreachable code, associated constant patterns and ref bindings. Nov 25, 2018
@davidtwco
Copy link
Member Author

Added a fix for #55401 too. Yet to look into the failures for the original PR issue.

@rust-highfive

This comment has been minimized.

@Xanewok
Copy link
Member

Xanewok commented Jan 2, 2019

I'm not sure I understand the fallout here.

   Compiling rls v1.31.6 (/home/xanewok/repos/rust/src/tools/rls)
error[E0621]: explicit lifetime required in the type of `msg`======> ] 245/246: rls(bin)
   --> src/tools/rls/src/server/mod.rs:185:21
    |
173 |       fn dispatch_message(&mut self, msg: &RawMessage) -> Result<(), jsonrpc::Error> {
    |                                           ----------- help: add explicit lifetime `'static` to the type of `msg`: `&'static server::message::RawMessage`
...
185 |                       <$n_action as LSPNotification>::METHOD => {
    |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lifetime `'static` required
...
259 | /         match_action!(
260 | |             msg.method;
261 | |             notifications:
262 | |                 notifications::Initialized,
...   |
287 | |                 requests::CodeLensRequest;
288 | |         );
    | |__________- in this macro invocation
error: aborting due to previous error

With $method being expanded to msg.method (which itself is of type String), we match on $method.as_str() and handle associated constants of type &'static str.

Indeed, matched value msg.method.as_str() will be of shorter lifetime (let's say &'a str) than the arms (associated constants of &'static str type) but I don't understand why that's an error. Shouldn't it be the other way around, that the match arms should live at least as long as the matched value? Am I missing something?

Xanewok added a commit to Xanewok/rls that referenced this pull request Jan 2, 2019
This exchanges a compilation error for a warning-but-future-error
but at least it compiles now and I'm not exactly sure how to fix it
without cloning those `METHOD` constants on every match (which is less
than ideal, to say the least).
@nikomatsakis
Copy link
Contributor

I filed #57280, as this seems like a bug (and a regression).

Xanewok added a commit to rust-lang/rls that referenced this pull request Jan 2, 2019
Xanewok added a commit to Xanewok/rust that referenced this pull request Jan 2, 2019
Breakage was due to rust-lang#55937,
which seemed to introduce a regression (tracked over at
rust-lang#57280).
@Xanewok Xanewok mentioned this pull request Jan 2, 2019
@davidtwco davidtwco deleted the issue-54943 branch January 3, 2019 14:46
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 5, 2019
Changes:
````
Update Clippy
Move TestFailures when collecting failures
Update languageserver-types to 0.51.1
update clippy hash and rustc_tools_util and use rustc_tools_util from crates.io
Work around rust-lang#55937
Update Clippy... again
Update Clippy
Update clippy
````
bors added a commit that referenced this pull request Jan 5, 2019
submodules: update clippy and rls

Fixes clippy toolstate

Changes:
````
Update to latest compiletest-rs release
add testcase for #3462
deps: bump rustc_tools_util version from 0.1.0 to 0.1.1 just in case...
rustc_tool_utils: fix failure to create proper non-repo version string when used in crates on crates.io, bump version
UI test cleanup: Extract ifs_same_cond tests
UI test cleanup: Extract for_kv_map lint tests
Fix test for #57250
Limit infinite_iter collect() check to known types
Some improvements to util documentation
Use hashset for name blacklist
Reformat random_state tests
Use node_id_to_type_opt instead of node_it_to_type in random_state
Check pattern equality while checking declaration equality
random_state lint
Use an FxHashSet for valid idents in documentation lint
Fix suggestion for unnecessary_ref lint
Update CONTRIBUTING.md for rustfix tests
Update .fixed files via update-references.sh
Run rustfix on first UI test
Use WIP branch for compiletest_rs
````

Also updates RLS and removes the patching of rustc_tool_utils from cargo.toml

RLS changes:
````
update clippy hash and rustc_tools_util and use rustc_tools_util from crates.io
Work around #55937
Update Clippy... again
Update Clippy
Update clippy
````
r? @oli-obk
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 7, 2019
Changes:
````
Update Clippy
Move TestFailures when collecting failures
Update languageserver-types to 0.51.1
update clippy hash and rustc_tools_util and use rustc_tools_util from crates.io
Work around rust-lang#55937
Update Clippy... again
Update Clippy
Update clippy
````
bors added a commit that referenced this pull request Jan 7, 2019
submodules: update clippy and rls

Fixes clippy toolstate

Changes:
````
Update to latest compiletest-rs release
add testcase for #3462
deps: bump rustc_tools_util version from 0.1.0 to 0.1.1 just in case...
rustc_tool_utils: fix failure to create proper non-repo version string when used in crates on crates.io, bump version
UI test cleanup: Extract ifs_same_cond tests
UI test cleanup: Extract for_kv_map lint tests
Fix test for #57250
Limit infinite_iter collect() check to known types
Some improvements to util documentation
Use hashset for name blacklist
Reformat random_state tests
Use node_id_to_type_opt instead of node_it_to_type in random_state
Check pattern equality while checking declaration equality
random_state lint
Use an FxHashSet for valid idents in documentation lint
Fix suggestion for unnecessary_ref lint
Update CONTRIBUTING.md for rustfix tests
Update .fixed files via update-references.sh
Run rustfix on first UI test
Use WIP branch for compiletest_rs
````

Also updates RLS and removes the patching of rustc_tool_utils from cargo.toml

RLS changes:
````
update clippy hash and rustc_tools_util and use rustc_tools_util from crates.io
Work around #55937
Update Clippy... again
Update Clippy
Update clippy
````
r? @oli-obk
davidtwco added a commit to davidtwco/rust that referenced this pull request Feb 6, 2019
This commit fixes a bug introduced by rust-lang#55937 which started checking user
type annotations for associated type patterns. Where lowering a
associated constant expression would previously return a
`PatternKind::Constant`, it now returns a `PatternKind::AscribeUserType`
with a `PatternKind::Constant` inside, this commit unwraps that to
access the constant pattern inside and behaves as before.
bors added a commit that referenced this pull request Feb 8, 2019
Lower constant patterns with ascribed types.

Fixes #57960.

This PR fixes a bug introduced by #55937 which started checking user
type annotations for associated type patterns. Where lowering a
associated constant expression would previously return a
`PatternKind::Constant`, it now returns a `PatternKind::AscribeUserType`
with a `PatternKind::Constant` inside, this PR unwraps that to
access the constant pattern inside and behaves as before.

r? @pnkfelix
pietroalbini pushed a commit to pietroalbini/rust that referenced this pull request Feb 17, 2019
This commit fixes a bug introduced by rust-lang#55937 which started checking user
type annotations for associated type patterns. Where lowering a
associated constant expression would previously return a
`PatternKind::Constant`, it now returns a `PatternKind::AscribeUserType`
with a `PatternKind::Constant` inside, this commit unwraps that to
access the constant pattern inside and behaves as before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants