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

Infer regions for opaque types in borrowck #67681

Merged
merged 25 commits into from
Feb 15, 2020

Conversation

matthewjasper
Copy link
Contributor

This is a step towards the goal of typeck not doing region inference.

The commits up to Arena allocate the result of mir_borrowck are various bug fixes and prerequisites.
The remaining commits move opaque type inference to borrow checking.

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 28, 2019
@rust-highfive

This comment has been minimized.

@Centril Centril added the F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` label Dec 28, 2019
src/librustc_typeck/impl_wf_check.rs Show resolved Hide resolved
src/librustc_mir/transform/check_unsafety.rs Show resolved Hide resolved
src/librustc/hir/lowering.rs Outdated Show resolved Hide resolved
src/librustc/hir/lowering.rs Outdated Show resolved Hide resolved
src/librustc/infer/opaque_types/mod.rs Outdated Show resolved Hide resolved
src/librustc_mir/build/mod.rs Outdated Show resolved Hide resolved
src/librustc_mir/borrow_check/type_check/mod.rs Outdated Show resolved Hide resolved
src/librustc_mir/borrow_check/type_check/mod.rs Outdated Show resolved Hide resolved
src/librustc/infer/opaque_types/mod.rs Outdated Show resolved Hide resolved
src/librustc_typeck/collect.rs Outdated Show resolved Hide resolved
@Centril Centril added F-impl_trait_in_bindings `#![feature(impl_trait_in_bindings)]` F-member_constraints `#[feature(member_constraints)]` labels Dec 28, 2019
@matthewjasper
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Dec 29, 2019

⌛ Trying commit e23a0cb45307a173427b9fcaefe648530e5f27c6 with merge 109224fdba977e11e6046e91fe34f9430fd0203a...

@bors
Copy link
Contributor

bors commented Dec 29, 2019

☀️ Try build successful - checks-azure
Build commit: 109224fdba977e11e6046e91fe34f9430fd0203a (109224fdba977e11e6046e91fe34f9430fd0203a)

@rust-timer
Copy link
Collaborator

Queued 109224fdba977e11e6046e91fe34f9430fd0203a with parent e0239b4, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 109224fdba977e11e6046e91fe34f9430fd0203a, comparison URL.

@matthewjasper
Copy link
Contributor Author

matthewjasper commented Dec 29, 2019

Perf is slightly green. That wasn't exactly expected, I'll investigate tomorrow.
edit: at least some of it's due to making fewer is_freeze queries.

desc { |tcx| "borrow-checking `{}`", tcx.def_path_str(key) }
cache_on_disk_if(tcx, _) { key.is_local() && tcx.is_closure(key) }
cache_on_disk_if { key.is_local() }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you caching non-closure keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it's no longer just closures that can have interesting borrow checking results. I didn't realize that this could depend on the query's result though, I can change it to that if that wouldn't cause problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can look at the values, yes. What's looking at non-closure borrow checking results now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

type_of for opaque types is looking at it.

src/librustc/arena.rs Outdated Show resolved Hide resolved
src/librustc_mir/borrow_check/region_infer/opaque_types.rs Outdated Show resolved Hide resolved
};

if !opaque_type_values.is_empty() {
err.note(&format!("Inferred opaque type values:\n{:#?}", opaque_type_values));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if we want to have tests which use this.

@bors
Copy link
Contributor

bors commented Jan 6, 2020

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

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.

OK, I did a first pass. Everything looks like it makes sense but I left two comments for what seem to be the most "load-bearing" parts of the PR that I want to re-read. Thanks for pushing on this, @matthewjasper!

// Map back to "concrete" regions so that errors in
// `infer_opaque_definition_from_instantiation` can show
// sensible region names.
let universal_concrete_type =
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what's going on here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a hopefully helpful comment to this function.

Some(opaque_defn_ty) => opaque_defn_ty,
};
debug!("opaque_defn_ty = {:?}", opaque_defn_ty);
let subst_opaque_defn_ty =
Copy link
Contributor

Choose a reason for hiding this comment

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

I should read this again.

@matthewjasper matthewjasper force-pushed the infer-regions-in-borrowck branch 2 times, most recently from b840ce6 to ed6cb3f Compare January 11, 2020 22:22
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2020-01-11T22:23:04.9229154Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-11T22:23:04.9241348Z ##[command]git config gc.auto 0
2020-01-11T22:23:04.9243900Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-11T22:23:04.9246017Z ##[command]git config --get-all http.proxy
2020-01-11T22:23:04.9248670Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67681/merge:refs/remotes/pull/67681/merge
---
2020-01-11T23:06:01.3849661Z 594 | |         })
2020-01-11T23:06:01.3849945Z 595 | |     }
2020-01-11T23:06:01.3850189Z     | |_____^
2020-01-11T23:06:01.3854401Z 
2020-01-11T23:06:01.3862386Z thread 'rustc' panicked at 'no errors encountered even though `delay_span_bug` issued', src/librustc_errors/lib.rs:346:17
2020-01-11T23:06:01.3872324Z 
2020-01-11T23:06:01.3878189Z error: internal compiler error: unexpected panic
2020-01-11T23:06:01.3882048Z 
2020-01-11T23:06:01.3931512Z note: the compiler unexpectedly panicked. this is a bug.
2020-01-11T23:06:01.3931512Z note: the compiler unexpectedly panicked. this is a bug.
2020-01-11T23:06:01.3931568Z 
2020-01-11T23:06:01.3932251Z note: we would appreciate a bug report: ***/blob/master/CONTRIBUTING.md#bug-reports
2020-01-11T23:06:01.3932316Z 
2020-01-11T23:06:01.3932637Z note: rustc 1.42.0-nightly (1bdc742a3 2020-01-11) running on x86_64-unknown-linux-gnu
2020-01-11T23:06:01.3932673Z 
2020-01-11T23:06:01.3933133Z note: compiler flags: -Z external-macro-backtrace -Z unstable-options -Z binary-dep-depinfo -Z force-unstable-if-unmarked -C opt-level=2 -C debuginfo=0 -C link-args=-Wl,-rpath,$ORIGIN/../lib -C prefer-dynamic -C llvm-args=-import-instr-limit=10 -C debug-assertions=n --crate-type lib
2020-01-11T23:06:01.3933262Z note: some of the compiler flags provided by cargo are hidden
2020-01-11T23:06:01.3933295Z 
2020-01-11T23:06:01.4467995Z error: could not compile `rustc`.
2020-01-11T23:06:01.4471299Z warning: build failed, waiting for other jobs to finish...
---
2020-01-11T23:08:46.0325432Z   local time: Sat Jan 11 23:08:46 UTC 2020
2020-01-11T23:08:46.3268921Z   network time: Sat, 11 Jan 2020 23:08:46 GMT
2020-01-11T23:08:46.3270528Z == end clock drift check ==
2020-01-11T23:08:47.2925811Z 
2020-01-11T23:08:47.3042653Z ##[error]Bash exited with code '1'.
2020-01-11T23:08:47.3080049Z ##[section]Starting: Checkout
2020-01-11T23:08:47.3081984Z ==============================================================================
2020-01-11T23:08:47.3082047Z Task         : Get sources
2020-01-11T23:08:47.3082117Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors
Copy link
Contributor

bors commented Jan 12, 2020

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

@matthewjasper
Copy link
Contributor Author

I've made some more changes that may affect performance
@bors try @rust-timer queue

* Use better span for member constraint errors
* Avoid a bad suggestion
* Don't report member constraint errors if we have other universal
  region errors.
Also correctly calculate what the upper bounds are.
This ensures that NLL will infer suitable values for regions in opaque
types when it's possible.
@matthewjasper
Copy link
Contributor Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Feb 14, 2020

📌 Commit d863978 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 14, 2020
@bors
Copy link
Contributor

bors commented Feb 15, 2020

⌛ Testing commit d863978 with merge 19288dd...

bors added a commit that referenced this pull request Feb 15, 2020
…omatsakis

Infer regions for opaque types in borrowck

This is a step towards the goal of typeck not doing region inference.

The commits up to `Arena allocate the result of mir_borrowck` are various bug fixes and prerequisites.
The remaining commits move opaque type inference to borrow checking.

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Feb 15, 2020

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing 19288dd to master...

@zg2t2cwz4d
Copy link

👌🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-impl_trait_in_bindings `#![feature(impl_trait_in_bindings)]` F-member_constraints `#[feature(member_constraints)]` F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Development

Successfully merging this pull request may close these issues.

8 participants