Add warning if a resolution failed #49542

GuillaumeGomez · 2018-03-31T12:50:32Z

oli-obk · 2018-03-31T14:40:19Z

Can you add a ui test showing the new warnings?

GuillaumeGomez · 2018-03-31T15:59:22Z

Good idea!

Mark-Simulacrum · 2018-04-01T19:08:59Z

src/bootstrap/test.rs

+
+    fn make_run(run: RunConfig) {
+        let compiler = run.builder.compiler(run.builder.top_stage, run.host);
+        run.builder.ensure(tool::Rustdoc {


No need for this as builder.rustdoc below will take care of it for you.

cmd.arg("--rustdoc-path").arg(builder.rustdoc(compiler.host)); on what might be 898 -- that call to rustdoc will auto-ensure it for you, so you shouldn't need this.

GuillaumeGomez · 2018-04-01T20:22:36Z

Updated (I also added some useful options to rustdoc).

oli-obk · 2018-04-01T22:15:55Z

src/test/rustdoc-ui/intra-links-warning.stderr

@@ -0,0 +1,6 @@
+warning: [src] cannot be resolved, ignoring it...


What's with this src thing?

That's actually a good question... Looking where it's coming from.

oli-obk · 2018-04-01T22:25:43Z

src/bootstrap/test.rs

@@ -514,6 +514,44 @@ impl Step for RustdocJS {
    }
 }

+#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)]
+pub struct RustdocUi {


This is a lot of code for very few tests. I was hoping we could somehow express this in the regular ui tests by inserting some flags via comments in the test?

As would i, but (as far as i know) there's no facility in compiletest to swap out which binary is called for a specific test - only which arguments are passed. In addition, we would have to tell bootstrap to have rustdoc ready for the UI tests, even if someone is trying to run a specific non-rustdoc test.

Plus, we wanted to introduce UI tests for rustdoc at some point anyway. >_> Having them in a separate test directory will help us when we add more over time.

We need to have a struct directly to bind the test folder path.

oli-obk · 2018-04-01T22:27:23Z

src/librustdoc/clean/mod.rs

@@ -1160,6 +1160,10 @@ enum PathKind {
    Type,
 }

+fn resolution_failure(cx: &DocContext, path_str: &str) {
+    cx.sess().warn(&format!("[{}] cannot be resolved, ignoring it...", path_str));


Are there spans available that we can use to report the warnings?

It'll be part of another pr. I think this one is already big enough.

oli-obk · 2018-04-01T22:28:47Z

src/librustdoc/core.rs

        ..config::basic_options().clone()
    };

    let codemap = Lrc::new(codemap::CodeMap::new(sessopts.file_path_mapping()));
-    let diagnostic_handler = errors::Handler::with_tty_emitter(ColorConfig::Auto,


What is the purpose of these changes?

Allow rustdoc output to be checked through UI test suite.

GuillaumeGomez · 2018-04-02T16:06:37Z

Fixed a few comments.

QuietMisdreavus · 2018-04-02T21:52:05Z

src/libstd/lib.rs

@@ -47,7 +47,7 @@
 //! development you may want to press the **[-]** button near the top of the
 //! page to collapse it into a more skimmable view.
 //!
-//! While you are looking at that **[-]** button also notice the **[src]**
+//! While you are looking at that **[-]** button also notice the `[src]`


It's probably worth changing the [-] references to also use backticks, just so they're all using the same style.

QuietMisdreavus · 2018-04-02T22:08:39Z

The rustdoc changes look good to me, but i'm less certain about the changes to bootstrap and compiletest. I'd like some extra eyes to look at those bits.

GuillaumeGomez · 2018-04-04T14:18:20Z

cc @oli-obk

Mark-Simulacrum · 2018-04-05T21:27:45Z

src/bootstrap/test.rs

@@ -514,6 +514,41 @@ impl Step for RustdocJS {
    }
 }

+#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)]
+pub struct RustdocUi {


This struct should be replaceable with a host_test! macro invocation I believe.

I tried to at first to avoid writing code by hand, but failed... I can't only rely on UI or rustdoc test suite, I need a mix of the two.

Mark-Simulacrum · 2018-04-05T21:29:09Z

src/tools/compiletest/src/main.rs

@@ -276,6 +276,8 @@ pub fn parse_config(args: Vec<String>) -> Config {
        ),
    };

+    let src_base = opt_path(matches, "src-base");
+    let run_ignored = matches.opt_present("ignored");


Avoiding these changes seems good for consistency's sake.

How would you do it?

Er, this file should be possible to leave as-is: both changes were simply extracted from struct's definition AFAICT.

Mark-Simulacrum · 2018-04-05T21:29:53Z

src/tools/compiletest/src/runtest.rs

-            rustc.args(&["-Z", "incremental-verify-ich"]);
-            rustc.args(&["-Z", "incremental-queries"]);
-        }
+        println!("{:?}", is_rustdoc);


Leftover debugging code?

Oops indeed!

oli-obk

ups, forgot to submit this review...

oli-obk · 2018-04-04T19:03:23Z

src/test/rustdoc-ui/intra-links-warning.rs

+// except according to those terms.
+
+#![deny(warnings)]
+


Why isn't this deny warnings failing the test?

It hasn't been implemented in rustdoc (yet?). I'll remove this line.

oli-obk · 2018-04-04T19:04:51Z

src/bootstrap/test.rs

+        } else {
+            vec!["-Crpath".to_string()]
+        };
+        if !is_rustdoc_ui {


Merge the if below into this one

oli-obk · 2018-04-04T19:06:45Z

src/bootstrap/test.rs

+    }
+
+    fn make_run(run: RunConfig) {
+        let compiler = run.builder.compiler(run.builder.top_stage, run.host);


You need to make this depend on rustdoc. Otherwise rustdoc might not be available when running the tests. A call to ensure should suffice

Apparently it's done correctly (or at least that's what I understood from @Mark-Simulacrum's comment).

Yes, this is fine as is, we depend on rustdoc below when we pass it's path.

GuillaumeGomez · 2018-04-07T16:42:44Z

Updated.

GuillaumeGomez · 2018-04-13T21:11:37Z

Anyone? cc @oli-obk @Mark-Simulacrum @QuietMisdreavus

Mark-Simulacrum · 2018-04-14T00:01:32Z

r=me on bootstrap changes

GuillaumeGomez · 2018-04-14T10:28:17Z

@bors: r=Mark-Simulacrum

bors · 2018-04-14T10:28:18Z

📌 Commit ae4c6d0 has been approved by Mark-Simulacrum

bors · 2018-04-14T15:42:55Z

🔒 Merge conflict

bors · 2018-04-15T18:16:29Z

⌛ Testing commit 4b7f4cc with merge f6d0bfd...

@QuietMisdreavus

…Mark-Simulacrum Add warning if a resolution failed r? @QuietMisdreavus

bors · 2018-04-15T21:18:33Z

💔 Test failed - status-appveyor

GuillaumeGomez · 2018-04-15T21:44:05Z

The appveyor tests output just end on running 692 tests. So I have no idea if it's a "normal" failure or not...

bors · 2018-04-16T07:54:41Z

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

kennytm · 2018-04-16T09:19:24Z

@GuillaumeGomez That looks like just a spurious 3-hour timeout. But now you need to rebase anyway...

GuillaumeGomez · 2018-04-17T08:43:06Z

Rebased, let's go again!

@bors: r+ p=11

bors · 2018-04-17T08:43:07Z

📌 Commit 05275da has been approved by GuillaumeGomez

bors · 2018-04-17T09:02:09Z

⌛ Testing commit 05275da with merge 8728c7a...

@QuietMisdreavus

…GuillaumeGomez Add warning if a resolution failed r? @QuietMisdreavus

bors · 2018-04-17T11:31:14Z

☀️ Test successful - status-appveyor, status-travis
Approved by: GuillaumeGomez
Pushing 8728c7a to master...

rust-highfive assigned QuietMisdreavus Mar 31, 2018

GuillaumeGomez force-pushed the intra-link-resolution-error branch 2 times, most recently from e507a56 to dc4a9ea Compare April 1, 2018 19:11

Mark-Simulacrum reviewed Apr 1, 2018

View reviewed changes

GuillaumeGomez force-pushed the intra-link-resolution-error branch from dc4a9ea to fb1aae2 Compare April 1, 2018 20:20

oli-obk reviewed Apr 1, 2018

View reviewed changes

GuillaumeGomez force-pushed the intra-link-resolution-error branch from fb1aae2 to efadab3 Compare April 2, 2018 15:39

pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 2, 2018

QuietMisdreavus reviewed Apr 2, 2018

View reviewed changes

GuillaumeGomez force-pushed the intra-link-resolution-error branch from efadab3 to 1833b19 Compare April 2, 2018 22:15

nrc mentioned this pull request Apr 3, 2018

Tracking issue for RFC 1946 - intra-rustdoc links #43466

Closed

27 tasks

Mark-Simulacrum reviewed Apr 5, 2018

View reviewed changes

GuillaumeGomez force-pushed the intra-link-resolution-error branch from 1833b19 to 46f62ba Compare April 5, 2018 22:37

oli-obk reviewed Apr 6, 2018

View reviewed changes

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 14, 2018

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 14, 2018

bors added a commit that referenced this pull request Apr 15, 2018

Auto merge of #49542 - GuillaumeGomez:intra-link-resolution-error, r=…

f6d0bfd

…Mark-Simulacrum Add warning if a resolution failed r? @QuietMisdreavus

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 15, 2018

kennytm 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 Apr 16, 2018

GuillaumeGomez added 6 commits April 16, 2018 23:33

Add warning if a resolution failed

035ec5b

Add error-format and color-config options to rustdoc

a6fefde

Add rustdoc-ui test suite

b2192ae

Fix nits

16a3938

Fix empty tests

a3ed2ab

Remove unwanted auto-linking and update

05275da

GuillaumeGomez force-pushed the intra-link-resolution-error branch from 4b7f4cc to 05275da Compare April 16, 2018 21:41

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 17, 2018

bors added a commit that referenced this pull request Apr 17, 2018

Auto merge of #49542 - GuillaumeGomez:intra-link-resolution-error, r=…

8728c7a

…GuillaumeGomez Add warning if a resolution failed r? @QuietMisdreavus

bors merged commit 05275da into rust-lang:master Apr 17, 2018

bors mentioned this pull request Apr 17, 2018

Remove uses of Build across Builder steps #49972

Merged

GuillaumeGomez deleted the intra-link-resolution-error branch April 17, 2018 14:12

QuietMisdreavus mentioned this pull request Aug 3, 2018

Stabilize --color and --error-format options in rustdoc #53003

Merged

pnkfelix mentioned this pull request Dec 3, 2018

NLL error on closure, but not on equivalent function #55526

Closed

cuviper mentioned this pull request Nov 5, 2024

Fix an extra newline in rendered std doc #132617

Merged

		@@ -0,0 +1,6 @@
		warning: [src] cannot be resolved, ignoring it...

Add warning if a resolution failed #49542

Add warning if a resolution failed #49542

Conversation

GuillaumeGomez commented Mar 31, 2018

oli-obk commented Mar 31, 2018

GuillaumeGomez commented Mar 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Apr 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Apr 2, 2018

Choose a reason for hiding this comment

QuietMisdreavus commented Apr 2, 2018

GuillaumeGomez commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Apr 7, 2018

GuillaumeGomez commented Apr 13, 2018

Mark-Simulacrum commented Apr 14, 2018

GuillaumeGomez commented Apr 14, 2018

bors commented Apr 14, 2018

bors commented Apr 14, 2018

bors commented Apr 15, 2018

bors commented Apr 15, 2018

GuillaumeGomez commented Apr 15, 2018

bors commented Apr 16, 2018

kennytm commented Apr 16, 2018

GuillaumeGomez commented Apr 17, 2018

bors commented Apr 17, 2018

bors commented Apr 17, 2018

bors commented Apr 17, 2018