Create new Error type to provide more information #28683

GuillaumeGomez · 2015-09-26T22:05:45Z

This PR solves the following issues (or at least help users to understand the problem):

#![crate_name = "b"]
#![crate_type = "rlib"]

pub fn his_function_has_a_very_long_name_and_should_make_cargo_doc_explodes_because_it_will_want_to_make_a_filename_with_it_in_excess_of_the_max_filename_length_for_most_filesystem_this_is_not_yet_long_enough_i_need_moreis_function_has_a_very_long_name_and_should_make_cargo_doc_explodes_because_it_will_want_to_make_a_filename_with_it_in_excess_of_the_max_filename_length_for_most_filesystem_this_is_not_yet_long_enough_i_need_more_() {}

#![crate_name = "b"]
#![crate_type = "rlib"]

pub struct his_function_has_a_very_long_name_and_should_make_cargo_doc_explodes_because_it_will_want_to_make_a_filename_with_it_in_excess_of_the_max_filename_length_for_most_filesystem_this_is_not_yet_long_enough_i_need_moreis_function_has_a_very_long_name_and_should_make_cargo_doc_explodes_because_it_will_want_to_make_a_filename_with_it_in_excess_of_the_max_filename_length_for_most_filesystem_this_is_not_yet_long_enough_i_need_more_;

For the maximum filename length chosen, @gkoz gave me this link.

rust-highfive · 2015-09-26T22:05:56Z

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

gkoz · 2015-09-26T22:11:44Z

For the maximum filename length chosen, @gkoz gave me this link.

For more context, Ubuntu's encrypted home directory uses ecryptfs and considering Ubuntu's popularity this is likely to bite someone.

GuillaumeGomez · 2015-09-26T22:16:18Z

Thanks for the precisions @gkoz !

nagisa · 2015-09-27T15:54:25Z

Such links should instead be hashed or otherwise transformed to adhere to the target filesystem requirements. Erroring here is second worst solution after just panicking.

GuillaumeGomez · 2015-09-27T17:42:09Z

I didn't know what solution to choose so I took a compromise. However, hashing is just as fine for me.

alexcrichton · 2015-09-28T17:10:03Z

Why did this come up? While something that seems like it could generate an error, it seems like this will never reasonably come up in practice and as a result the extra code needed to handle this isn't worth it.

kali · 2015-09-29T14:10:24Z

It came up while I was working on the OpenCV bindings.

thread '' panicked at 'failed to generate documentation: File name too long (os error 63)', ../src/librustdoc/lib.rs:291

I had to generate very long names to get unambiguous names for C functions that wrap C++ methods. In the end, hiding these function from the documentation felt right, so I am not dependent on a fix for the actual issue.

That said panicking is terrible, because it does not even give the actual problematic name. (I was not aware functions had their pages, so I was only paying attention to types names, not freestanding functions names). A clean error message would have helped me in this instance.

alexcrichton · 2015-09-29T16:39:09Z

Thanks for the info @kali! I think the error handling in librustdoc could definitely use some improvement, and in this case I think the best solution is to just present the error better.

@GuillaumeGomez I don't think printing the error or returning a blank error is the right thing to do here, but rather this should be propagated upwards to get printed out at a later time. In theory a bigger infrastructure is also needed for attaching context to errors as it's propagated up the stack (e.g. what Cargo does), but it may be somewhat involved to add. I'd be fine opening an issue if this is getting a little too large for this PR!

alexcrichton · 2015-09-30T19:09:59Z

As a status update, some discussion on IRC lead to the conclusion that the best way to handle this was to continue to propagate the error and have it displayed in a more reasonable fashion. E.g. not via printing and returning an error with an empty message.

GuillaumeGomez · 2015-09-30T19:51:54Z

@alexcrichton: Now the returned error is more complete, is it what you wanted ? Also, there is still the weird panic, do you want me to try to take a look in the same PR ?

alexcrichton · 2015-10-01T16:51:49Z

src/librustdoc/html/render.rs

@@ -143,6 +144,33 @@ impl Impl {
    }
 }

+#[derive(Debug)]
+pub struct IoError {
+    filename: String,


This should be stored as a PathBuf as it's what file names are represented by

GuillaumeGomez · 2015-10-05T20:23:24Z

@alexcrichton: I updated the code. Now we have a difference between a full librustdoc::Error and a simple wrapped io::Error.

alexcrichton · 2015-10-06T23:42:08Z

src/librustdoc/html/render.rs

+                                                filename: Some(filename.to_owned()) }));
+                match render(dst, self, &item, true) {
+                    Ok(ret) => Ok(ret),
+                    Err(e) => Err(Error { error: e, filename: Some(String::new()) }),


This has the same problem as before where a blank filename is used to generate a suboptimal error message. This should just be able to use try!.

Also, if you don't have the From implementation above, how many instances of try! are there that no longer work? It seems like this should be one of the few sources of io::Error -> Error conversions

alexcrichton · 2015-10-08T16:23:09Z

Looking over this again it feels a little too ad-hoc to me. I think that if we're gonna go the route of having a more full-blown error with more contextual information that we'll not want to stop halfway and just add the extra information at only one location. I think removing the From implementation will be a good indicator of what call sites need to be fixed up, and ideally there's no From implementation at all (as it requires losing contextual information).

GuillaumeGomez · 2015-10-08T22:47:26Z

So I changed a bit how errors are returned. Now what kind of information do you want in the error ? You told me context information, but it has more than one meaning for me so could you precise your thoughts on this please ?

And of course, if I forgot anything, please add it ! :)

alexcrichton · 2015-10-09T17:57:53Z

src/librustdoc/html/render.rs

+}
+
+impl Error {
+    pub fn from(e: io::Error) -> Error {


I think this is basically the previous iteration but in a slightly more verbose fashion due to this. Basically all errors in rustdoc happen when working on some file, so if we want to attach the filename to the errors it should basically always be available to do so. Can you look into making filename not optional?

It's not possible to do this (take a look at build_index and write_shared functions). I'll add it as much places as possible but not all.

alexcrichton · 2015-10-12T02:39:03Z

@GuillaumeGomez I think this is definitely possible to add everywhere, even the build_index and write_shared functions. All errors will originate because of some filename in rustdoc. The usage in build_index should be .unwrap() because a Write into a Cursor<Vec<u8>> will never fail.

GuillaumeGomez · 2015-10-12T06:22:26Z

In this case yes, we can do it. Thanks for the tip !

alexcrichton · 2015-10-14T17:35:16Z

src/librustdoc/html/render.rs

+    }
+}
+
+#[macro_export]


This macro shouldn't need to be reexported

GuillaumeGomez · 2015-10-14T20:13:50Z

And done ! Bonus : we don't have the second weird error message. :)

GuillaumeGomez · 2015-10-16T15:49:40Z

@alexcrichton: Is anything missing ?

alexcrichton · 2015-10-16T17:17:52Z

Looks good to me, thanks! Could you squash and update the PR title/description for what this currently does?

alexcrichton · 2015-10-16T22:45:23Z

@bors: r+ 42c01d7

@gkoz

This PR solves the following issues (or at least help users to understand the problem): ```Rust #![crate_name = "b"] #![crate_type = "rlib"] pub fn his_function_has_a_very_long_name_and_should_make_cargo_doc_explodes_because_it_will_want_to_make_a_filename_with_it_in_excess_of_the_max_filename_length_for_most_filesystem_this_is_not_yet_long_enough_i_need_moreis_function_has_a_very_long_name_and_should_make_cargo_doc_explodes_because_it_will_want_to_make_a_filename_with_it_in_excess_of_the_max_filename_length_for_most_filesystem_this_is_not_yet_long_enough_i_need_more_() {} ``` ```Rust #![crate_name = "b"] #![crate_type = "rlib"] pub struct his_function_has_a_very_long_name_and_should_make_cargo_doc_explodes_because_it_will_want_to_make_a_filename_with_it_in_excess_of_the_max_filename_length_for_most_filesystem_this_is_not_yet_long_enough_i_need_moreis_function_has_a_very_long_name_and_should_make_cargo_doc_explodes_because_it_will_want_to_make_a_filename_with_it_in_excess_of_the_max_filename_length_for_most_filesystem_this_is_not_yet_long_enough_i_need_more_; ``` For the maximum filename length chosen, @gkoz gave me [this link](http://unix.stackexchange.com/a/32834).

bors · 2015-10-16T23:28:16Z

⌛ Testing commit 42c01d7 with merge 20a6938...

bors · 2015-10-17T02:03:31Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

rust-highfive assigned brson Sep 26, 2015

brson assigned alexcrichton and unassigned brson Sep 30, 2015

GuillaumeGomez force-pushed the rustdoc_error branch from 4143ae2 to f2a8d9c Compare September 30, 2015 19:29

alexcrichton reviewed Oct 1, 2015
View reviewed changes

GuillaumeGomez force-pushed the rustdoc_error branch from d76654d to 89b553b Compare October 6, 2015 06:24

alexcrichton reviewed Oct 6, 2015
View reviewed changes

GuillaumeGomez force-pushed the rustdoc_error branch from 89b553b to 5dbe7e0 Compare October 7, 2015 18:38

alexcrichton reviewed Oct 9, 2015
View reviewed changes

GuillaumeGomez force-pushed the rustdoc_error branch from 6445230 to 4366059 Compare October 10, 2015 20:41

GuillaumeGomez force-pushed the rustdoc_error branch 3 times, most recently from 6de69bc to 6567b00 Compare October 12, 2015 22:01

alexcrichton reviewed Oct 14, 2015
View reviewed changes

src/librustdoc/html/render.rs

}

}

#[macro_export]

Copy link

Member

alexcrichton Oct 14, 2015

Choose a reason for hiding this comment

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

This macro shouldn't need to be reexported

GuillaumeGomez force-pushed the rustdoc_error branch 2 times, most recently from d66e3a1 to 15aa338 Compare October 14, 2015 20:13

Improve error handling by providing Error struct with Path information

42c01d7

GuillaumeGomez force-pushed the rustdoc_error branch from 15aa338 to 42c01d7 Compare October 16, 2015 22:21

GuillaumeGomez changed the title ~~Add checkup on filename length and print out an error message~~ Create new Error type to provide more information Oct 16, 2015

bors merged commit 42c01d7 into rust-lang:master Oct 17, 2015

GuillaumeGomez deleted the rustdoc_error branch October 17, 2015 09:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create new Error type to provide more information #28683

Create new Error type to provide more information #28683

GuillaumeGomez commented Sep 26, 2015

rust-highfive commented Sep 26, 2015

gkoz commented Sep 26, 2015

GuillaumeGomez commented Sep 26, 2015

nagisa commented Sep 27, 2015

GuillaumeGomez commented Sep 27, 2015

alexcrichton commented Sep 28, 2015

kali commented Sep 29, 2015

alexcrichton commented Sep 29, 2015

alexcrichton commented Sep 30, 2015

GuillaumeGomez commented Sep 30, 2015

alexcrichton Oct 1, 2015

GuillaumeGomez commented Oct 5, 2015

alexcrichton Oct 6, 2015

alexcrichton commented Oct 8, 2015

GuillaumeGomez commented Oct 8, 2015

alexcrichton Oct 9, 2015

GuillaumeGomez Oct 10, 2015

alexcrichton commented Oct 12, 2015

GuillaumeGomez commented Oct 12, 2015

alexcrichton Oct 14, 2015

GuillaumeGomez commented Oct 14, 2015

GuillaumeGomez commented Oct 16, 2015

alexcrichton commented Oct 16, 2015

alexcrichton commented Oct 16, 2015

bors commented Oct 16, 2015

bors commented Oct 17, 2015

Create new Error type to provide more information #28683

Create new Error type to provide more information #28683

Conversation

GuillaumeGomez commented Sep 26, 2015

rust-highfive commented Sep 26, 2015

gkoz commented Sep 26, 2015

GuillaumeGomez commented Sep 26, 2015

nagisa commented Sep 27, 2015

GuillaumeGomez commented Sep 27, 2015

alexcrichton commented Sep 28, 2015

kali commented Sep 29, 2015

alexcrichton commented Sep 29, 2015

alexcrichton commented Sep 30, 2015

GuillaumeGomez commented Sep 30, 2015

alexcrichton Oct 1, 2015

Choose a reason for hiding this comment

GuillaumeGomez commented Oct 5, 2015

alexcrichton Oct 6, 2015

Choose a reason for hiding this comment

alexcrichton commented Oct 8, 2015

GuillaumeGomez commented Oct 8, 2015

alexcrichton Oct 9, 2015

Choose a reason for hiding this comment

GuillaumeGomez Oct 10, 2015

Choose a reason for hiding this comment

alexcrichton commented Oct 12, 2015

GuillaumeGomez commented Oct 12, 2015

alexcrichton Oct 14, 2015

Choose a reason for hiding this comment

GuillaumeGomez commented Oct 14, 2015

GuillaumeGomez commented Oct 16, 2015

alexcrichton commented Oct 16, 2015

alexcrichton commented Oct 16, 2015

bors commented Oct 16, 2015

bors commented Oct 17, 2015