Don't use QueryNormalizer to normalize in rustdoc

[compiler-errors]

The QueryNormalizer, which is what rustdoc was using to normalize its types, has a note:

/// N.B., this will *eventually* be the main means of
/// normalizing, but for now should be used only when we actually
/// know that normalization will succeed, since error reporting
/// and other details are still "under development".

In Rustdoc, this is currently not true -- it will gladly accept programs that aren't completely correct (see issue that this PR fixes #102827). This causes an ICE currently, because the normalizer doesn't currently handle normalization ambiguity correctly.

So let's use another normalization scheme, namely fully_normalize, to normalize types in rustdoc. It's slightly different in semantics, like how it doesn't allow escaping bound vars, so I added an extra normalization call in one place where we're pretty blatantly skipping binders. More normalize calls may need to be added in other places before we call skip_binder.

---

Alternatively, I could just revert 7808f69, which caused this ICE. It's not clear that that's a better solution than this, since @lcnr's PR actually did signal a shortcoming in the current normalize call -- we don't really have a way to signal failures due to ambiguity, since right now all we return is a Result<T, NoSolution>.

[rust-highfive]

r? @notriddle

(rust-highfive has picked a reviewer for you, use r? to override)

[jyn514]

So let's use another normalization scheme, namely fully_normalize, to normalize types in rustdoc. It's slightly different in semantics, like how it doesn't allow escaping bound vars, so I added an extra normalization call in one place where we're pretty blatantly skipping binders. More normalize calls may need to be added in other places before we call skip_binder.

@compiler-errors how does this interact with #82692 ? Does it still immediately give a fatal error when the type length limit is reached?

Anyway, using a normalize method that panics less often is great, thanks :) rustdoc really wants to be able to recover from any kind of error when normalizing. Is the difference between these documented somewhere?

[jyn514]

It's slightly different in semantics, like how it doesn't allow escaping bound vars, so I added an extra normalization call in one place where we're pretty blatantly skipping binders. More normalize calls may need to be added in other places before we call skip_binder.

Wait, I don't understand how these are related. Why would normalizing in one place affect the results of normalizing in another? How is skip_binders related?

[compiler-errors]

@jyn514, so I guess first I wanted to mention that traits::fully_normalize will ICE if you give it something with escaping bound vars. That is, if we call binder.skip_binders() somewhere and pass that value to the normalize call, then it'll ICE if any lifetimes reference vars in the binder. That means I had to add a if value.has_escaping_bound_vars() { return None; } check in normalize, which means we potentially skip normalizing in some cases.

One specific case where this can be frequently seen is clean_fn_decl_with_args -- it takes a PolyFnSig, which is a Binder<FnSig>. So when we skip the binder of the fn signature, grab its output type, and try to clean (thus normalize) that type, if the output type has escaping bound vars (e.g. for<'a> fn(&'a Ty) -> &'a <Ty as Trait>::Assoc), then we must skip normalizing it to prevent an ICE.

However, we can normalize a type and its binder at the same type. By normalizing the whole signature first, we normalize the associated type within the binder its debruijn indices actually point to. So we end up doing something like for<'a> fn(&'a Ty) -> &'a <Ty as Trait>::Assoc -> for<'a> fn(&'a Ty) -> &'a Concrete. Then later when we skip the binder to and clean the output type, we already have the normalized &'a Concrete, and it doesn't matter if we skip normalizing the type again.

---

The underlying issue is that this normalize call should probably not exist in clean_middle_ty at all, because it's not properly keeping track of binders that we skip when we recursively clean inner types -- but instead normalization should happen as close to the place where we originally got type data from astconv. For example, if we call tcx.fn_sig, tcx.type_of, etc. on a def-id, then we should be immediately normalizing this, however that requires inserting a lot more normalize calls around rustdoc, which is tedious.

[compiler-errors]

how does this interact with #82692 ? Does it still immediately give a fatal error when the type length limit is reached?

Can you explain a bit more? That's a tracking issue, not a specific error I can test.

[jyn514]

how does this interact with #82692 ? Does it still immediately give a fatal error when the type length limit is reached?

Can you explain a bit more? That's a tracking issue, not a specific error I can test.

#102038 is the test; currently it gives a fatal error on overflow instead of silently continuing.

traits::fully_normalize will ICE if you give it something with escaping bound vars. That is, if we call binder.skip_binders() somewhere and pass that value to the normalize call, then it'll ICE if any lifetimes reference vars in the binder.

Ok. How hard would that be to change? I'd rather not try to work around it in rustdoc.

[compiler-errors]

Ok. How hard would that be to change? I'd rather not try to work around it in rustdoc.

Pretty hard. It's only coincidental that the old normalization call rustdoc was using didn't ICE with escaping bound vars, since that normalize call has its own hack -- however, rustdoc is fundamentally using the normalize call in a way that it shouldn't, so I do think it's rustdoc's responsibility to work around it.

[jyn514]

What does "ways it shouldn't" mean here?

[compiler-errors]

"shouldn't" means that normalize should only be called on things that are properly wrapped in binders, so we actually keep track of the late-bound lifetimes within a type. When you skip a binder, it throws away the list of bound vars that the inner type references, so it's not possible to match up where a late-bound region is actually coming from.

It's like going from for<'a> fn(&'a ()) to fn(&'a ()) -- it confuses normalization machinery to not know exactly what the late-bound regions are, and we've added assertions around the normalization machinery to control for this. That's the ICE I mentioned above that I needed to add an extra check to avoid.

[bors]

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

[compiler-errors]

The change in #102858 is a bit more conservative approach if we don't want to move to using fully_normalize here and have to worry about binders...

[notriddle]

For example, if we call tcx.fn_sig, tcx.type_of, etc. on a def-id, then we should be immediately normalizing this, however that requires inserting a lot more normalize calls around rustdoc, which is tedious.

How bad would it be to create wrappers for those functions in rustdoc that immediately normalize the result, then change all the other spots to call the wrappers?

[jyn514]

(I'm still not convinced that's the best approach; if you can give me a day or two to look at this I'll try and get back to you by then.)

[compiler-errors]

I actually agree with @notriddle that that would be the ideal solution, but that's quite a lot of work.

Anything we get from rustc_middle should be normalized like as soon as we substitute it, if we're trying to do normalization the same way as the rest of the compiler expects it to be done.

[oli-obk]

I like making progress here more than what was done in #102858

[compiler-errors]

@oli-obk, we still have legitimate issues with this bug!() assertion:

rust/compiler/rustc_trait_selection/src/traits/query/normalize.rs

Line 256 in 542febd

bug!("unexpected ambiguity: {:?} {:?}", c_data, result);

... in non-rustdoc code that this PR does not fix (#103181), though. I actually am not really a fan of this approach compared to #102858, which is both simpler and more general 🤔

[oli-obk]

Hmm, but that one is already erroring, so we could even just delay span bug and return NoSolution at the bug! site.

Tho we could go with your other PR and track the other future rustdoc cleanups proposed here in an issue.

I don't have a strong opinion, so let's close this one and I'llvm approve the other one, the impl lgtm.

[compiler-errors]

yeet