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

Use PredicateObligations instead of Predicates #69745

Merged
merged 2 commits into from
Apr 10, 2020

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Mar 5, 2020

Keep more information about trait binding failures. Use more specific spans by pointing at bindings that introduce obligations.

Subset of #69709.

r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 5, 2020
@estebank
Copy link
Contributor Author

estebank commented Mar 5, 2020

cc @nikomatsakis

Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

I need to move relevant comments from the original PR, and review the code that is different.

.map(|pred| traits::Obligation::new(cause.clone(), self.param_env, pred))
.zip(predicates.spans.into_iter())
.map(|(pred, span)| {
let cause = self.cause(traits::BindingObligation(def_id, span));
Copy link
Member

Choose a reason for hiding this comment

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

Before I forget, cc @nikomatsakis on the ItemObligation vs BindingObligation split, and the name of the latter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, the name BindingObligation doesn't say that much to me, and the rustdoc on it is not very precise:

Like ItemObligation, but with extra detail on the source of the obligation.

I presume that it's meant to track the precise bound that caused this obligation? In which case I think the name is "decent", but I'd probably just call it WhereClause or something. I don't think the suffix "Obligation" is adding much here.

Copy link
Contributor Author

@estebank estebank left a comment

Choose a reason for hiding this comment

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

Adding some links to relevant comments in the original PR.

.map(|pred| traits::Obligation::new(cause.clone(), self.param_env, pred))
.zip(predicates.spans.into_iter())
.map(|(pred, span)| {
let cause = self.cause(traits::BindingObligation(def_id, span));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/librustc_infer/traits/util.rs Show resolved Hide resolved
src/librustc_typeck/collect.rs Outdated Show resolved Hide resolved
@bors

This comment has been minimized.

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 28, 2020
@estebank estebank force-pushed the predicate-obligations-3 branch 2 times, most recently from 538a8e3 to cd37a6e Compare April 6, 2020 05:25
Comment on lines 4 to 8
LL | trait Foo {
| --------- required by `Foo`
| --------- required by this bound in `Foo`
...
LL | where &'a T : Foo,
| ^^^ cannot infer type for reference `&'a T`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a few cases like this one where the inference error itself is already incorrect and the wording change makes them slightly worse in my eyes.

They seem rare enough that we don't need to deal with them immediately, but we should probably account for them.

Comment on lines 148 to 153
--> $DIR/issue-20413.rs:8:36
|
LL | trait Foo {
| --------- required by `Foo`
| --------- required by this bound in `Foo`
...
LL | impl<T> Foo for T where NoData<T>: Foo {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another case.

@estebank estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 6, 2020
@nikomatsakis
Copy link
Contributor

FYI: I plan to review this this week.

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.

The code seems good, but I'm not sure what I think of the final output.

@@ -3,6 +3,8 @@ error[E0277]: `MyNotSync` cannot be shared between threads safely
|
LL | fn is_sync<T: Sync>() {}
| ------- ---- required by this bound in `is_sync`
| |
| required by a bound in `is_sync`
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 a bit confused by these diffs. That is -- I'm not quite sure what this label is trying to tell me. I guess what was happening before is that we had a span for is_sync but lacked more precise information, so we got no label, but now we get a label?

Nonetheless, I'm not sure this is a net improvement -- the label that tells you which bound definitely seems good, and this new label kind of distracts from that.

Would it be possible to get more precise information to help us in tracking which label is causing the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was born out of #69745 (comment), where the small separation between the two spans made @eddyb confused for a short moment. I thought that adding some label to the span might make it clearer in all cases that we're pointing at two different things. Another alternative would be to only point at the fn ident if it is in a different line to the bound.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, that change is in its own commit, so you can look at the diff without it.

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 have changed the second commit to check if both spans are in the same line, and if they are, to not display the span pointing at is_sync.

@bors

This comment has been minimized.

@estebank estebank force-pushed the predicate-obligations-3 branch 2 times, most recently from dccbca2 to ece8320 Compare April 8, 2020 21:34
@estebank
Copy link
Contributor Author

estebank commented Apr 9, 2020

@eddyb this PR has a lot of potential to bitrot and is blocking the PR to maintain chain of obligations. @nikomatsakis signed off on the code changes but not on the new output. I think the changes in the second commit should be non-controversial.

@eddyb
Copy link
Member

eddyb commented Apr 9, 2020

@bors r=nikomatsakis,eddyb

@bors
Copy link
Contributor

bors commented Apr 9, 2020

📌 Commit d605a9d has been approved by nikomatsakis,eddyb

@estebank
Copy link
Contributor Author

Dist extended stage1 (x86_64-apple-darwin)
Building stage1 tool cargo (x86_64-apple-darwin)
dyld: Library not loaded: /usr/lib/libcurl.4.dylib
  Referenced from: /Users/runner/runners/2.165.2/work/1/s/build/x86_64-apple-darwin/stage0/bin/cargo
  Reason: image not found
command did not execute successfully: "/Users/runner/runners/2.165.2/work/1/s/build/x86_64-apple-darwin/stage0/bin/cargo" "build" "--target" "x86_64-apple-darwin" "-Zbinary-dep-depinfo" "-j" "4" "--release" "--locked" "--color" "always" "--manifest-path" "/Users/runner/runners/2.165.2/work/1/s/src/tools/cargo/Cargo.toml" "--features" "rustc-workspace-hack/all-static" "--message-format" "json-render-diagnostics"
expected success, got: signal: 6
failed to run: /Users/runner/runners/2.165.2/work/1/s/build/bootstrap/debug/bootstrap dist
Build completed unsuccessfully in 1:11:06

@bors retry

@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 Apr 10, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 10, 2020
Rollup of 9 pull requests

Successful merges:

 - rust-lang#69745 (Use `PredicateObligation`s instead of `Predicate`s)
 - rust-lang#70938 (Add ThreadSanitizer test case)
 - rust-lang#70973 (Use forward traversal for unconditional recursion lint)
 - rust-lang#70978 (compiletest: let config flags overwrite -A unused)
 - rust-lang#70979 (Follow up on BTreeMap comments)
 - rust-lang#70981 (Rearrange BTreeMap::into_iter to match range_mut.)
 - rust-lang#70985 (Clean up E0512 explanation)
 - rust-lang#70988 (Setup the `@rustbot prioritize` command)
 - rust-lang#70991 (fix rustc-dev-guide's url in src/librustc_codegen_ssa)

Failed merges:

r? @ghost
@bors bors merged commit 1fe86f4 into rust-lang:master Apr 10, 2020
matthiaskrgr added a commit to matthiaskrgr/rust-clippy that referenced this pull request Apr 10, 2020
matthiaskrgr added a commit to matthiaskrgr/rust-clippy that referenced this pull request Apr 10, 2020
bors added a commit to rust-lang/rust-clippy that referenced this pull request Apr 11, 2020
bors added a commit to rust-lang/rust-clippy that referenced this pull request Apr 11, 2020
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 12, 2020
Changes:
````
Allow UUID style formatting for `inconsistent_digit_grouping` lint
rustup rust-lang#70986
rustup rust-lang#69745
Rustup to rust-lang#70913
compare with the second largest instead of the smallest variant
Revert "Downgrade new_ret_no_self to pedantic"
Check for clone-on-copy in argument positions
Check fn header along with decl when suggesting to implement trait
Downgrade implicit_hasher to pedantic
Move cognitive_complexity to nursery
Run fmt and update test
Use int assoc consts in MANUAL_SATURATING_ARITHMETIC
Use int assoc consts in checked_conversions lint
Use primitive type assoc consts in more tests
Use integer assoc consts in more lint example code
Don't import primitive type modules
Use assoc const NAN for zero_div_zero lint
Fix float cmp to use assoc fxx::EPSILON
Fix NAN comparison lint to use assoc NAN
Refine lint message.
Lint on opt.as_ref().map(|x| &**x).
Include OpAssign in suspicious_op_assign_impl
result_map_or_into_option: fix syntax error in example
result_map_or_into: fix dogfood_clippy error => {h,l}int
CONTRIBUTING.md: fix broken triage link
result_map_or_into_option: fix `cargo dev fmt --check` errors
result_map_or_into_option: move arg checks into tuple assignment
result_map_or_into_option: add `opt.map_or(None, |_| Some(y))` test
result_map_or_into_option: destructure lint tuple or return early
result_map_or_into_option: add good and bad examples
result_map_or_into_option: explicitly note absence of known problems
Downgrade new_ret_no_self to pedantic
Downgrade unreadable_literal to pedantic
Update CONTRIBUTING.md
Rename rustc -> rustc_middle in doc links
result_map_or_into_option: add lint to catch manually adpating Result -> Option
Move matches test in matches module
Run update_lints
Make lint modules private
Don't filter lints in code generation functions
Build lint lists once and the reuse them to update files
Get rid of Lint::is_internal method
Clean up update_lints
Downgrade inefficient_to_string to pedantic
Downgrade trivially_copy_pass_by_ref to pedantic
Downgrade let_unit_value to pedantic
````
Fixes rust-lang#70993
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 13, 2020
submodules: update clippy from d342cee to af5940b

Changes:
````
Allow UUID style formatting for `inconsistent_digit_grouping` lint
rustup rust-lang#70986
rustup rust-lang#69745
Rustup to rust-lang#70913
compare with the second largest instead of the smallest variant
Revert "Downgrade new_ret_no_self to pedantic"
Check for clone-on-copy in argument positions
Check fn header along with decl when suggesting to implement trait
Downgrade implicit_hasher to pedantic
Move cognitive_complexity to nursery
Run fmt and update test
Use int assoc consts in MANUAL_SATURATING_ARITHMETIC
Use int assoc consts in checked_conversions lint
Use primitive type assoc consts in more tests
Use integer assoc consts in more lint example code
Don't import primitive type modules
Use assoc const NAN for zero_div_zero lint
Fix float cmp to use assoc fxx::EPSILON
Fix NAN comparison lint to use assoc NAN
Refine lint message.
Lint on opt.as_ref().map(|x| &**x).
Include OpAssign in suspicious_op_assign_impl
result_map_or_into_option: fix syntax error in example
result_map_or_into: fix dogfood_clippy error => {h,l}int
CONTRIBUTING.md: fix broken triage link
result_map_or_into_option: fix `cargo dev fmt --check` errors
result_map_or_into_option: move arg checks into tuple assignment
result_map_or_into_option: add `opt.map_or(None, |_| Some(y))` test
result_map_or_into_option: destructure lint tuple or return early
result_map_or_into_option: add good and bad examples
result_map_or_into_option: explicitly note absence of known problems
Downgrade new_ret_no_self to pedantic
Downgrade unreadable_literal to pedantic
Update CONTRIBUTING.md
Rename rustc -> rustc_middle in doc links
result_map_or_into_option: add lint to catch manually adpating Result -> Option
Move matches test in matches module
Run update_lints
Make lint modules private
Don't filter lints in code generation functions
Build lint lists once and the reuse them to update files
Get rid of Lint::is_internal method
Clean up update_lints
Downgrade inefficient_to_string to pedantic
Downgrade trivially_copy_pass_by_ref to pedantic
Downgrade let_unit_value to pedantic
````
Fixes rust-lang#70993

r? @Dylan-DPC
@ecstatic-morse
Copy link
Contributor

FYI, this seems to have slowed down clean check builds of the futures crate by 8%.

@eddyb
Copy link
Member

eddyb commented Apr 17, 2020

Oh dear, it makes the stress test for #65510 way worse. That test isn't entirely artificial btw.
It's simulating a set of real-world situations where traits and associated types are heavily used.

Also, 5% on serde itself would be enough to warrant a look.

Comment on lines +300 to +302
.into_iter()
.map(|o| o.predicate)
.collect::<Vec<_>>(),
Copy link
Member

@eddyb eddyb Apr 17, 2020

Choose a reason for hiding this comment

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

Ahh I wonder if it's stuff like this - this is very wasteful and unlikely to be needed.

Comment on lines +117 to +118
let obligations: Vec<_> =
predicates.into_iter().map(|predicate| predicate_obligation(predicate, None)).collect();
Copy link
Member

Choose a reason for hiding this comment

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

Again, this is Vec -> Vec, which we should avoid ever doing. I wish I spotted this earlier, oof.

let predicates = obligations.iter().map(|obligation| obligation.predicate).collect();
let implied_obligations = traits::elaborate_predicates(tcx, predicates);
let implied_obligations = implied_obligations.map(|pred| {
let implied_obligations = traits::util::elaborate_obligations(tcx, obligations.clone());
Copy link
Member

Choose a reason for hiding this comment

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

At a glance this seems wasteful but it's not worse than it was before. Presumably we can avoid this clone altogether, but I haven't checked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only reason I didn't strip them outright was because of retain calls in elaborate_predicates, but if we can change all cases of Vec<Pred> argument types into impl Iterator<Item = Pred>, we should be faster than before.

Comment on lines -300 to +302
util::elaborate_predicates(tcx, unnormalized_env.caller_bounds.to_vec()).collect();
util::elaborate_predicates(tcx, unnormalized_env.caller_bounds.to_vec())
.map(|obligation| obligation.predicate)
.collect();
Copy link
Member

Choose a reason for hiding this comment

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

Tempted to try and find every allocation in the trait system and rewrite it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you going to be doing it or should I try and do some of it now?

Copy link
Member

Choose a reason for hiding this comment

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

If you want to play around with it, feel free to! I was mostly considering taking this over in case you don't have time for it.

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 won't have time to look at elaborate_obligations, which is the one that might have the biggest impact, but I can have a small PR shortly where I'm replacing a bunch of these arguments with impl Iterator<Item = PredicateObligation> so that it becomes clearer where we're collecting multiple times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at this in closer detail, I think I might be able to simplify this a bit.

@estebank
Copy link
Contributor Author

@eddyb @ecstatic-morse #71268 seems to get us closer to the previous baseline but I also found a couple of other improvements I might be able to make.

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 19, 2020
…ikomatsakis

 Maintain chain of derived obligations

When evaluating the derived obligations from super traits, maintain a
reference to the original obligation in order to give more actionable
context in the output.

Continuation (and built on) rust-lang#69745, subset of rust-lang#69709.

r? @eddyb
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 27, 2020
Remove some `Vec` allocations to improve performance

This claws back most of the performance lost in rust-lang#69745.
r? @eddyb
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request May 5, 2020
Changes:
````
Allow UUID style formatting for `inconsistent_digit_grouping` lint
rustup rust-lang/rust#70986
rustup rust-lang/rust#69745
Rustup to rust-lang/rust#70913
compare with the second largest instead of the smallest variant
Revert "Downgrade new_ret_no_self to pedantic"
Check for clone-on-copy in argument positions
Check fn header along with decl when suggesting to implement trait
Downgrade implicit_hasher to pedantic
Move cognitive_complexity to nursery
Run fmt and update test
Use int assoc consts in MANUAL_SATURATING_ARITHMETIC
Use int assoc consts in checked_conversions lint
Use primitive type assoc consts in more tests
Use integer assoc consts in more lint example code
Don't import primitive type modules
Use assoc const NAN for zero_div_zero lint
Fix float cmp to use assoc fxx::EPSILON
Fix NAN comparison lint to use assoc NAN
Refine lint message.
Lint on opt.as_ref().map(|x| &**x).
Include OpAssign in suspicious_op_assign_impl
result_map_or_into_option: fix syntax error in example
result_map_or_into: fix dogfood_clippy error => {h,l}int
CONTRIBUTING.md: fix broken triage link
result_map_or_into_option: fix `cargo dev fmt --check` errors
result_map_or_into_option: move arg checks into tuple assignment
result_map_or_into_option: add `opt.map_or(None, |_| Some(y))` test
result_map_or_into_option: destructure lint tuple or return early
result_map_or_into_option: add good and bad examples
result_map_or_into_option: explicitly note absence of known problems
Downgrade new_ret_no_self to pedantic
Downgrade unreadable_literal to pedantic
Update CONTRIBUTING.md
Rename rustc -> rustc_middle in doc links
result_map_or_into_option: add lint to catch manually adpating Result -> Option
Move matches test in matches module
Run update_lints
Make lint modules private
Don't filter lints in code generation functions
Build lint lists once and the reuse them to update files
Get rid of Lint::is_internal method
Clean up update_lints
Downgrade inefficient_to_string to pedantic
Downgrade trivially_copy_pass_by_ref to pedantic
Downgrade let_unit_value to pedantic
````
Fixes #70993
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