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

Fix invalid bounds string generation in rustdoc #58894

Merged
merged 3 commits into from Apr 6, 2019

Conversation

Projects
None yet
10 participants
@GuillaumeGomez
Copy link
Member

commented Mar 3, 2019

Fixes #58737.

Very weird and I'm not sure this is the best fix around. However, trying to fix it beforehand seems overly complicated compared to the gain (in clean, it wouldn't change anything since we have to return something so that wouldn't work, and in hir, I'm afraid I'd break something else for very little gain).

Also, I wasn't able to make a small code to reproduce the issue. The only way to test is to document crossbeam directly and check the Scope struct...

r? @QuietMisdreavus

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

The bug seems to be introduced when re-exporting the struct (Scope is actually defined in crossbeam_utils), and i bet the PhantomData<&'env mut &'env ()> has something to do with the erroneous bound.

I was able to reproduce the error with the following code:

use std::marker::PhantomData;

pub struct Scope<'env> {
    _marker: PhantomData<&'env mut &'env ()>,
}

A downstream crate that re-exports the Scope struct will create the erroneous bound. I wonder if this is something we should deal with inside the compiler? @rust-lang/compiler Do inferred region bounds somehow make it into a crate's metadata? Rustdoc is seeing an erroneous where 'env: 'env on the above struct, and we're looking for ways to filter that out in the docs.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:invalid-lifetime-bounds branch from 774fb17 to 869ab87 Mar 5, 2019

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

Your test doesn't properly exercise the bug, since it primarily appears when the struct is re-exported. However, i would still like to see input from @rust-lang/compiler in case it can be fixed on their end.

@eddyb

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

Do inferred region bounds somehow make it into a crate's metadata?

Well... they must be, mustn't they? Otherwise how do you rely on them cross-crate?

It may be possible that in rustdoc you're using predicates_of (which includes inferred_outlives_of) when you want explicit_predicates_of:

/// Returns the predicates written explicit by the user.
[] fn explicit_predicates_of: ExplicitPredicatesOfItem(DefId)
-> Lrc<ty::GenericPredicates<'tcx>>,
/// Returns the inferred outlives predicates (e.g., for `struct
/// Foo<'a, T> { x: &'a T }`, this would return `T: 'a`).
[] fn inferred_outlives_of: InferredOutlivesOf(DefId) -> Lrc<Vec<ty::Predicate<'tcx>>>,

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

Do inferred region bounds somehow make it into a crate's metadata?

Well... they must be, mustn't they? Otherwise how do you rely on them cross-crate?

I was under the assumption that they could be re-calculated from the definition, but i guess not.

It may be possible that in rustdoc you're using predicates_of (which includes inferred_outlives_of) when you want explicit_predicates_of:
rust/src/librustc/ty/query/mod.rs

It looks like we are using predicates_of when cleaning the AST (and inlining items). I feel like switching to explicit_predicates_of would fix both this issue and #56331.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

JFYI we could probably screen out 'a: 'a bounds from the inferred bounds list, although it doesn't sound to me like the "right" fix here necessarily -- that would be done by adding a check roughly here to see whether r == outlived_region, I think

@Centril

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

Ping from triage, @GuillaumeGomez :)

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2019

It looks like we are using predicates_of when cleaning the AST (and inlining items). I feel like switching to explicit_predicates_of would fix both this issue and #56331.

Well, it panics when I tried to switch so not sure about that... Investigating.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:invalid-lifetime-bounds branch from 869ab87 to ddd034a Mar 31, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2019

I changed a few things around but we now have a new issue: for non local elements, it just doesn't work. Any idea on this @eddyb?

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Mar 31, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (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.
travis_time:end:22bc8a3c:start=1554039234938787025,finish=1554039310187377572,duration=75248590547
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
Setting environment variables from .travis.yml
---

[00:03:30] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:31] tidy error: /checkout/src/test/rustdoc/useless_lifetime_bound.rs: missing trailing newline
[00:03:32] some tidy checks failed
[00:03:32] 
[00:03:32] 
[00:03:32] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:03:32] 
[00:03:32] 
[00:03:32] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:32] Build completed unsuccessfully in 0:00:47
[00:03:32] Build completed unsuccessfully in 0:00:47
[00:03:32] Makefile:67: recipe for target 'tidy' failed
[00:03:32] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:26400256
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sun Mar 31 13:38:52 UTC 2019
---
travis_time:end:199745e8:start=1554039533390029443,finish=1554039533395503719,duration=5474276
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:024de792
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0b3e7aae
travis_time:start:0b3e7aae
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:08395634
$ dmesg | grep -i kill

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)

@eddyb

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

I was under the assumption that they could be re-calculated from the definition, but i guess not.

Yeah, the problem is that the definition is sort of lost (part of why rustdoc needs two codepaths for everything...), and only some information of note is kept.

Although specifically in this case, enough information might be there, but it would be a bit of waste to recompute it each time.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

Fixed the issue!

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

I also added the test from #59033.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

@estebank

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

📌 Commit c966c45 has been approved by estebank

Centril added a commit to Centril/rust that referenced this pull request Apr 5, 2019

Rollup merge of rust-lang#58894 - GuillaumeGomez:invalid-lifetime-bou…
…nds, r=estebank

Fix invalid bounds string generation in rustdoc

Fixes rust-lang#58737.

Very weird and I'm not sure this is the best fix around. However, trying to fix it beforehand seems overly complicated compared to the gain (in `clean`, it wouldn't change anything since we **have to** return something so that wouldn't work, and in `hir`, I'm afraid I'd break something else for very little gain).

Also, I wasn't able to make a small code to reproduce the issue. The only way to test is to document `crossbeam` directly and check the `Scope` struct...

r? @QuietMisdreavus

@Centril Centril referenced this pull request Apr 5, 2019

Merged

Rollup of 6 pull requests #59741

bors added a commit that referenced this pull request Apr 5, 2019

Auto merge of #59741 - Centril:rollup-3us4b8q, r=Centril
Rollup of 6 pull requests

Successful merges:

 - #58894 (Fix invalid bounds string generation in rustdoc)
 - #59599 (Updated RELEASES.md for 1.34.0)
 - #59624 (SGX target: Use linker option to avoid code CGU assignment kludge)
 - #59696 (Remove invalid assertion back:🔗:from add_upstream_rust_crates().)
 - #59707 (Add missing tryfrom example)
 - #59727 (wasi: Use shared API for preopened fds)

Failed merges:

r? @ghost

@bors bors merged commit c966c45 into rust-lang:master Apr 6, 2019

@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:invalid-lifetime-bounds branch Apr 6, 2019

let hir_id = tcx.hir().as_local_hir_id(def_id).unwrap();
let hir_id = match tcx.hir().as_local_hir_id(def_id) {
Some(hir_id) => hir_id,
None => return tcx.predicates_of(def_id),

This comment has been minimized.

Copy link
@eddyb

eddyb Apr 6, 2019

Member

This is another change to rustc_typeck that wasn't explicitly approved (the comment thread doesn't make it clear that a hack was added outside of rustdoc), can we please not do this? I'll create a PR reverting these, but I feel like we have a larger process break issue...

Yes, I didn't see this notification until now, but please ping me elsewhere instead of merging typeck hacks like this.

This comment has been minimized.

Copy link
@eddyb

eddyb Apr 6, 2019

Member

Not to mention this is wrong for cross-crate items, my expectation was that explicit_predicates_of was available cross-crate.

This comment has been minimized.

Copy link
@Centril

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Apr 7, 2019

Author Member

I did ping the compiler team so I thought you'd see the notification, my bad.

This comment has been minimized.

Copy link
@estebank

estebank Apr 7, 2019

Contributor

Talked with Eddy on chat. My bad as the previous seemed to make sense on isolation. I'll defer to Eddy's assessment on typeck code in the future.

This comment has been minimized.

Copy link
@eddyb

eddyb Apr 8, 2019

Member

@GuillaumeGomez Being notified isn't enough, we need to acknowledge it too.
With @nikomatsakis less active lately (and also on PTO more recently), a bit more care should be taken.

This comment has been minimized.

Copy link
@eddyb

eddyb Apr 8, 2019

Member

Opened #59789.

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Apr 8, 2019

Author Member

I didn't approved the PR nor asked anyone (except the compiler team) to review it. I approve for more care to be taken but I feel like taking the blame for an error I didn't make. :-/

This comment has been minimized.

Copy link
@oli-obk

oli-obk Apr 8, 2019

Contributor

This is a process failure, not your failure! We'll figure out how to prevent such things happening in the future.

This comment has been minimized.

Copy link
@estebank

estebank Apr 12, 2019

Contributor

@GuillaumeGomez The one at fault here was me, not you, as I approved this change thinking it looked reasonable without being my main area of expertise, I should have deferred to @eddyb's or @nikomatsakis' judgement on this case.

bors added a commit that referenced this pull request Apr 9, 2019

Auto merge of #59789 - eddyb:typeck-reverts, r=<try>
Revert two unapproved changes to rustc_typeck.

There was a breakdown in process (#59004 (comment), #58894 (comment)) and two changes were made to `rustc_typeck`'s "collect" queries, for rustdoc, that were neither needed *nor* correct.
I'm reverting them here, and will fix up rustdoc *somehow*, if necessary.

cc @rust-lang/compiler How do we ensure this doesn't happen again?

r? @nikomatsakis or @oli-obk

bors added a commit that referenced this pull request Apr 10, 2019

Auto merge of #59789 - eddyb:typeck-reverts, r=<try>
Revert two unapproved changes to rustc_typeck.

There was a breakdown in process (#59004 (comment), #58894 (comment)) and two changes were made to `rustc_typeck`'s "collect" queries, for rustdoc, that were neither needed *nor* correct.
I'm reverting them here, and will fix up rustdoc *somehow*, if necessary.

cc @rust-lang/compiler How do we ensure this doesn't happen again?

r? @nikomatsakis or @oli-obk
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.