Stream: rustdoc

Topic: -Z normalize-docs #81091


view this post on Zulip Joshua Nelson (Apr 13 2021 at 14:13):

I'm unclear on many parts yet but I guess most importantly unclear on with this change, how would the rustdoc feature work after I had managed to fix this...? would the docs just have the full, unbound path like <Source as SelectDsl>::Output for the type Select<Source> in your minimization https://github.com/rust-lang/rust/issues/79459#issuecomment-734984867 ?

@Joonas Koivunen Yes - the idea is that if it can't be normalized, it should just fallback to showing the original output

view this post on Zulip Joshua Nelson (Apr 13 2021 at 14:14):

if you look at the rustdoc code it actually does that already for you: https://github.com/jyn514/rust/blob/0679a4cd93db7dfd33b45cd5bd6be52a0f5e894a/src/librustdoc/clean/mod.rs#L1544

view this post on Zulip Joshua Nelson (Apr 13 2021 at 14:14):

the problem is that normalize is literally calling process::exit so it never gets that far

view this post on Zulip Joshua Nelson (Apr 13 2021 at 14:15):

if you change normalize to return an error instead, basically the only change needed is to turn map() into and_then() and ignore the overflow error like all the other errors

view this post on Zulip Joshua Nelson (Apr 13 2021 at 14:20):

thank you for working on this! :heart:

view this post on Zulip Joonas Koivunen (Apr 13 2021 at 14:55):

Great! At least I understood that correctly. So far I've rebased your modifications and yes, like I mentioned, it was quite interesting to see the conflict reasons (there were huge changes merged between now and when you started -- though I guess this is normal).

I should probably push that version in case I never get this done. I still think it's a good possibility since there's just so much ground to cover, or at least it seems like that.

However, the rebased or incomplete version wasn't too clear for me (given the new codebase of rustc), so I started again from a clean master and now I pretty much agree with your changes, except the ones in rustc_trait_selection/src/traits/error_reporting/{mod,suggestions}.rs, but perhaps those can be discussed later.

view this post on Zulip Joshua Nelson (Apr 13 2021 at 15:00):

tbh I did those changes while sleeping on a couch in a friends apartment so they may not be my best work :joy:

view this post on Zulip Joonas Koivunen (Apr 13 2021 at 15:09):

sorry, with unclear I just meant the reasoning(s) behind each change, except for the type system requiring it :) I guess I'll continue working to better trying to understand the second commit. I do understand why the obligation needs to go into the OverflowError, but this commit changed the build errors so drasticly it's going to take some time.

also I linked the rebased version to the issue rust#81091

view this post on Zulip Joshua Nelson (Apr 13 2021 at 15:27):

I do understand why the obligation needs to go into the OverflowError, but this commit changed the build errors so drasticly it's going to take some time.

yeah this was the point at which I gave up :laughing: if you find an easier way to do this without storing the obligation I'd definitely be interested

view this post on Zulip Joshua Nelson (Apr 13 2021 at 15:34):

now I pretty much agree with your changes, except the ones in rustc_trait_selection/src/traits/error_reporting/{mod,suggestions}.rs, but perhaps those can be discussed later.

the reason these don't exit on overflow is this only happens if an error is already going to be reported

view this post on Zulip Joshua Nelson (Apr 13 2021 at 15:34):

so the overflow is from generating suggestions, it's unrelated to the actual problem in the code

view this post on Zulip Joonas Koivunen (Apr 13 2021 at 16:45):

my thinking was the previous implementation would had stopped on the first overflow, if such would had been found when generating help. however I don't now if such a "double-fault" condition was ever possible. either way, that is probably something which should be visible when it comes to testing this..

view this post on Zulip Joshua Nelson (Apr 13 2021 at 16:50):

well yes, but IMO that's the wrong behavior - it means you see an error that's unrelated to what actually went wrong

view this post on Zulip Joonas Koivunen (Apr 13 2021 at 16:52):

that is an excellent point, now that I think about it :)

view this post on Zulip Joshua Nelson (Apr 19 2021 at 18:15):

hey @Joonas Koivunen had you made any progress with this? Happy to help if you got stuck :)

view this post on Zulip Joonas Koivunen (Apr 19 2021 at 18:33):

no progress to report, been a bit busy with daily $work. I'll try to gather my notes/thoughts for tomorrow if I can get some concrete questions.

view this post on Zulip Joonas Koivunen (Apr 20 2021 at 18:36):

All righty.. sorry for the delay @Joshua Nelson . Been a bit busier than expected as deadlined $work has mysteriously appeared :)

So what I was last looking was the necessity of the obligation you introduced in the "everything is broken" commit into rustc_middle::traits::select::OverflowError. This cannot work as is because either the Obligation would have to move from rustc_infer::traits to the middle, or be handled in some other way.

In order the understand the "other possible ways", I started looking at the api surface of rustc_trait_selection and it's quite huge, almost all items seem to be public and I guess I was last left at this point.

view this post on Zulip Joonas Koivunen (Apr 20 2021 at 18:39):

Looking back into the hints provided on rust#81091 I know realize that perhaps there isn't any reason to expand this outwards from the given rustc_trait_selection::traits::{select,projection}. From the hint in issue description:

until outside of rustc_trait_selection::traits::{select, project}, at which point they should be emitted.

emit probably means reporting the error and stopping, similar to what rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt::evaluate_obligation_no_overflow used to do (it's now removed in the branch)?

view this post on Zulip Joonas Koivunen (Apr 20 2021 at 18:43):

After writing up this status report I feel like I've been sidetracked by following and pondering all of the possible OverflowError returns and instead (or in addition) I should try approach this by going top-down from the API surface of rustc_trait_selection::traits::{select,project}::* which I guess I've ended up looking up the least. Though there has certainly been enough of rustc dev guide to read here as well.

Would the "top-down" sound sensible idea to you, or had you gotten some better idea in the meantime?

Also, sorry for this many messages kind of comms. I'm on +03:00 timezone so we've got a bit of tz difference. I'll be a bit afk now but will check your reply tomorrow at the latest.

view this post on Zulip Joshua Nelson (Apr 20 2021 at 18:58):

Oh no worries, I have a tendency to ramble in chats myself :laughing: I like the idea of doing this top-down. I'd start with where rustdoc calls normalize() - I think that's a query? So any type it returns has to be available in rustc_middle.

view this post on Zulip Joonas Koivunen (Apr 21 2021 at 06:49):

Ah yes, thanks to your note about it being most likely being a query, I think something clicked -- or at least it feels that way. The query abstraction and the way it's populated are definitely something I haven't yet come across elsewhere not to even mention the incremental side of it.

view this post on Zulip Joshua Nelson (Apr 21 2021 at 09:19):

yes so queries are weird because they can be declared and implemented separately

view this post on Zulip Joshua Nelson (Apr 21 2021 at 09:19):

you don't have to worry about the incremental part for this, but as long as the argument types and the return types are defined in rustc_middle the funciton itself can be defined anywhere

view this post on Zulip Joshua Nelson (Apr 21 2021 at 09:20):

search for provide(

view this post on Zulip Joonas Koivunen (Apr 21 2021 at 11:18):

Trying to move the obligation now, got ... 73 errors next, all OverflowError without 'tcx, T. Feels weird writing such a change, as normally I'd want to know very deeply the query system and the incremental parts before moving such a struct into the return value. Lets see what'll happen :)

view this post on Zulip Joonas Koivunen (Apr 21 2021 at 16:31):

today I started moving Obligation et. al. over to rustc_middle, still a lot of errors. the biggest one is that rustc_middle::traits::SelectionError derives TypeFoldable and Lift. now that I've added SelectionError::Overflow(PredicateOverflow<'tcx>) which is rustc_middle::traits::select::OverflowError<'tcx, ty::Predicate<'tcx>>, I'm not seeing a trivial path to impl the traits by using the derives.

I'm not really certain on any if these, but if you would happen to have already explored the TypeFoldable and Lift derives, is there any point in deriving it for the smaller types which most likely do not have anything to do with typefolding or even lifting (since they are 'static). Also, it'd seem that deriving doesn't even work for some reason.

view this post on Zulip Joshua Nelson (Apr 21 2021 at 16:36):

hmm maybe this is a sign this is the wrong approach

view this post on Zulip Joshua Nelson (Apr 21 2021 at 16:37):

do you remember why I needed to make Obligation part of OverflowError in the first place?

view this post on Zulip Joonas Koivunen (Apr 22 2021 at 06:30):

do you remember why I needed to make Obligation part of OverflowError

not so much as "remember" but my hunch is that if the resolution goes from terminating version to result returning version, the existing call sites which can't handle the OverflowError will need to have the previous reporting, which used to happen like:

evaluate_root_obligation (compiler/rustc_trait_selection/src/traits/select/mod.rs)
-> evaluate_predicates_recursively
-> evaluate_predicate_recursively
-> check_recursion_limit
-> report_overflow_error (compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs)

where we'll have:

match self.query_mode {
    TraitQueryMode::Standard => {
        self.infcx().report_overflow_error(error_obligation, true);
    }
    TraitQueryMode::Canonical => {
        // in master this doesn't have error_obligation
        return Err(OverflowError(error_obligation.to_owned()));
    }
}

So I'd imagine those callsites which cannot handle the Overflow would in future call some new helper to re-evaluate the root_obligation, just called from some unwrap_or_else(|| <here>) (the new helper would have to unconditionally terminate).

view this post on Zulip Joonas Koivunen (Apr 22 2021 at 06:41):

hmm maybe this is a sign this is the wrong approach

I think a "no-op" typefoldable at least might be easy to implement, as there'd be none to typefold over but I do agree with this hunch as:

I rebased the branch yesterday, and now the HEAD reflects the current state: https://github.com/koivunej/rust/commit/e14bb0517b6da4e4bd4973bfeb339d522bc864c7

view this post on Zulip Joonas Koivunen (Apr 22 2021 at 08:18):

I rebased the branch yesterday, and now the HEAD reflects the current state: https://github.com/koivunej/rust/commit/e14bb0517b6da4e4bd4973bfeb339d522bc864c7

I did reorderings to make the diff more readable and force pushed: https://github.com/koivunej/rust/commit/557f1a029fd87ffad89d6705a233aa83d6eacee6

view this post on Zulip Joshua Nelson (Apr 22 2021 at 13:51):

So I'd imagine those callsites which cannot handle the Overflow would in future call some new helper to re-evaluate the root_obligation, just called from some unwrap_or_else(|| <here>) (the new helper would have to unconditionally terminate).

yes, that sounds like a good plan. and that would mean we don't have to add the obligation directly to the error

view this post on Zulip Joshua Nelson (Apr 22 2021 at 13:51):

cc @Matthew Jasper - does that sound like about what you were expecting?

view this post on Zulip Joonas Koivunen (Apr 22 2021 at 15:28):

And the issue was rust#81091 (for some reason I start to remember this series of digits :D)

view this post on Zulip Matthew Jasper (Apr 22 2021 at 17:00):

hmm, I don't think that I was expecting overflow errors to be returned from queries.


Last updated: Oct 11 2021 at 22:34 UTC