Remove MaybeForgetReturn
suggestion
#137303
Open
+4
−83
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#115196 implemented a suggestion to add a missing
return
when there is an ambiguity error, when that ambiguity error could be constrained by the return type of the function.I initially reviewed it and thought it could be useful; however, looking back at that code now, I feel like it's a bit too much of a hack to be worth keeping around in typeck, especially given how rare it's expected to fire in practice. This is especially true because it depends on
StashKey::MaybeForgetReturn
, which is only stashed when we have Sized obligation ambiguity errors. Let's remove it for now.I'd like to note that it's basically impossible to get this suggestion to apply in its current state except for what I'd consider somewhat artificial examples, involving no generic trait bounds. For example, it's not triggered for:
Nor is it triggered for:
It's basically only triggered iff there's only one ambiguity error on the type, which is
Sized
.Generally, suggesting something that affects control flow is a pretty dramatic suggestion; therefore, both the accuracy and precision of this diagnostic should be pretty high.
One other, somewhat unrelated observation is that this might be using stashed diagnostics incorrectly (or at least unnecessarily). Stashed diagnostics are used when error detection is fragmented over several major stages of the compiler, like a parse or resolver error which later can be recovered in typeck. However, this one is a bit different since it is fully handled within typeck -- perhaps that suggests that if this were to be reimplemented, it wouldn't need to be so complicated of an implementation.