feat: dedup "the size for values ... cannot be known" error

[Ezrashaw]

Fixes #105753

Two points on the UI updates:
- most are de-duplicating trait errors (which I assume come from instantiate_binder_with_fresh_vars), this is invisible to the user regardless.
- tests/ui/unsized-locals/unsized-exprs.rs: seems to make sense within unsized_fn_params.

out of date

[rustbot]

r? @cjgillot

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

[cjgillot]

Not checking whether the type is sized when we do require it is not a solution to extra diagnostics. I'd rather have a solution based on tweaking the diagnostic output, not a change to the type-checking logic.

[Ezrashaw]

I realize now that my impl is not going to work. Some advice on what can be done would be nice. Although I don't think that diagnostic de-duplication will work, it'd still require changes to typechecking. What would probably be best is if we kept the local variable error and attached a note saying that the bound value is unsized.

EDIT: how can I check if an expression is bound to a local variable?

[Ezrashaw]

cc @compiler-errors

Would wrapping the sized check in:

let parent = tcx.hir().parent_id(expr.hir_id);
let parent = tcx.hir().get_parent(parent);
if !matches!(parent, hir::Node::Local(..)) {
    // ...
}

work? All local variables are still guaranteed to be sized. Is there some way to downgrade this error with require_type_is_sized_deferred?

[Ezrashaw]

@cjgillot @compiler-errors I've implemented what I said above.

[compiler-errors]

I'm still pretty uncomfortable with this approach, and still think @cjgillot's initial comment is valid. I still think this needs to be focused on diagnostic deduplication via post-processing, rather than relaxing where a type is required to be Sized in the type checker.

[Ezrashaw]

@compiler-errors Do you have any advice on where to start with that? I'm not entirely sure what you mean by "diagnostics deduplication" (I associate that with -Zdeduplicate-diagnostics, which I don't think is applicable here). This requires more than just deduping errors, all three errors are completely valid and can exist in isolation.

[compiler-errors]

Sorry, not sure if I can give much advice. By diagnostics deduplication, I mean the general approach to maintaining additional state that can be used to soundly defuse reporting errors for Sized obligations that don't add additional information after one has been reported.

We do that in various other ways in other places in the compiler (unfortunately, in pretty ad-hoc ways as well, though, so not sure if you can adopt any machinery to help you with this). Fixing this probably requires some novel brainstorming, and it's not something I've given much though towards, since these redundant errors issues are typically P-low and haven't really caught my interest recently.

At the end of the day, registering fewer Sized obligations is pretty risky, and something I'm not certain that we should be doing just to make a few error outputs shorter.

[Ezrashaw]

@compiler-errors

don't add additional information

I note that emphasis, in this case it is definitely a redundant error which is completely unnecessary and confusing.

By diagnostics deduplication, I mean the general approach to maintaining additional state that can be used to soundly defuse reporting errors for Sized obligations.

Sorry for the excess clarification but this can be done with downgrade_to_delayed_bug for one of the errors when we're sure that other errors have been emitted?

Also (and I know my current approach isn't ideal) could you point to a concrete example where this code causes unsoundness or ICEs?

[lrh2000]

Maybe a better approach is to determine whether def-site (i.e. defining a function) errors imply ref-site (i.e. referencing a function) errors. To be more specific, in the original issue (#105753), the first error (dyn T is unsized), referred to as a def-site error, actually implies the third error (dyn T + 'static is unsized), referred to as a ref-site error.

After checking the code, it seems that currently we do have similar logic in order to remove fulfillment errors that are implied by others.

rust/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

Lines 429 to 458 in 8a7ca93

	// We do this in 2 passes because we want to display errors in order, though
	// maybe it *is* better to sort errors by span or something.
	let mut is_suppressed = vec![false; errors.len()];
	for (_, error_set) in error_map.iter() {
	// We want to suppress "duplicate" errors with the same span.
	for error in error_set {
	if let Some(index) = error.index {
	// Suppress errors that are either:
	// 1) strictly implied by another error.
	// 2) implied by an error with a smaller index.
	for error2 in error_set {
	if error2.index.map_or(false, |index2| is_suppressed[index2]) {
	// Avoid errors being suppressed by already-suppressed
	// errors, to prevent all errors from being suppressed
	// at once.
	continue;
	}
	if self.error_implies(error2.predicate, error.predicate)
	&& !(error2.index >= error.index
	&& self.error_implies(error.predicate, error2.predicate))
	{
	info!("skipping {:?} (implied by {:?})", error, error2);
	is_suppressed[index] = true;
	break;
	}
	}
	}
	}
	}

However, its current capability is limited by removing duplicate errors at only the same span. It should be reasonable as it is not easy to determine whether errors are really duplicate if they happen at different places. But back in this issue, we know that def-site unsized errors and ref-site unsized errors are actually related, so we should be confident at removing the ref-site errors (once ref-site errors are indeed implied by call-site errors?).

In fact, it also seems that we do not have enough information (in the FulfillmentError structure) to relate def-site errors and call-site errors now. If it is the case, maybe we need additionally to record the function's DefId in the obligation cause (after breaking down SizedReturnType into SizedReturnTypeDefSite and SizedReturnTypeRefSite?).

rust/compiler/rustc_middle/src/traits/mod.rs

Lines 304 to 305 in 553ecbe

	/// Return type must be `Sized`.
	SizedReturnType,

(Just a few of my own thoughts. I'm not very familiar with related code so things could be wrong.. cc @compiler-errors as he have contributed much in the related files.)

[Ezrashaw]

@lrh2000 I think that you're right, I'll probably try to implement this. The only thing might be around how this interacts with unsized_locals and related features which might break the def-site implies call-site idea.

[Ezrashaw]

See https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/dedup.20sized.20errors.20PR/near/350151854 for rationale behind latest attempt.