Conversation
rustbot
added
S-waiting-on-review
|
I am not very confident about fallout bless's: 994cca7 |
This comment has been minimized.
This comment has been minimized.
|
this fails to bootstrap, probably something related to specialization 🤔 😁 please minimize and look into why |
994cca7 to
36d20a8
Compare
This comment has been minimized.
This comment has been minimized.
|
@rustbot author |
rustbot
added
S-waiting-on-author
36d20a8 to
083fa89
Compare
rustbot
added
S-waiting-on-review
| } | ||
| } | ||
|
|
||
| fn has_distinct_impl_candidates( |
There was a problem hiding this comment.
maybe instead have a try_merge_impl_candidates which returns a Option<CanonicalResponse<I>> and is None if there are no impls. That might make fn merge_trait_candidates nicer to read 🤔
fn has_distinct_impl_candidates is definitely kinda meh as it checks trait_is_marker in whcih case we allow distinct candidates.
also, please add comments where applicable
the behavior itself lgtm
There was a problem hiding this comment.
Added try_merge_impl_candidates here: 76f0d90, and updated the docs. Thanks.
4fee497 to
76f0d90
Compare
76f0d90 to
53e9bd8
Compare
| self.flounder(&candidates).map(|r| (r, None)) | ||
| match self.try_merge_impl_candidates(trait_def_id, &candidates) { | ||
| Some(response) => Ok((response, Some(proven_via))), | ||
| None => self.flounder(&candidates).map(|r| (r, None)), | ||
| } |
There was a problem hiding this comment.
hmm, can you do another change to the structure to make it easier to read (for me)
use
self.try_merge_candidates(&alias_bounds) {
Ok((response, Some(TraitGoalProvenVia::AliasBound)))
} else {
Ok((self.bail_with_ambiguity(&alias_bounds), None))
};in the TraitGoalProvenVia::ParamEnv arm. Though, maybe change this pattern to use a new function, like merge_candidates_or_bail_with_ambiguity(&alias_bounds, TraitGoalProvenVia::AliasBound).
| if !self.cx().trait_is_marker(trait_def_id) { | ||
| let impl_count = candidates | ||
| .iter() | ||
| .filter(|candidate| matches!(candidate.source, CandidateSource::Impl(_))) | ||
| .count(); | ||
| let has_builtin_impl = candidates | ||
| .iter() | ||
| .any(|candidate| matches!(candidate.source, CandidateSource::BuiltinImpl(_))); | ||
| if impl_count + usize::from(has_builtin_impl) > 1 { | ||
| return None; | ||
| } | ||
| } | ||
|
|
||
| self.try_merge_candidates(candidates).map(|(response, _)| response) |
There was a problem hiding this comment.
could you change this to
if is_marker || candidates
.iter()
.all(|candidate| matches!(candidate.source, CandidateSource::BuiltinImpl(_))) {
self.try_merge_candidates(candidates).map(|(response, _)| response)
} else if candidates.len() > 1 {
None
} else {
candidates.first().copied() or sth like this
}
53e9bd8 to
6302baf
Compare
| trait_def_id: I::TraitId, | ||
| candidates: &[Candidate<I>], | ||
| ) -> Result<(CanonicalResponse<I>, Option<TraitGoalProvenVia>), NoSolution> { | ||
| match self.try_merge_impl_candidates(trait_def_id, candidates) { |
There was a problem hiding this comment.
can you add a debug assert that all candidates are actually impl candidates here?
| let only_global_where_bounds = !candidates.is_empty() | ||
| && candidates | ||
| .iter() | ||
| .all(|c| matches!(c.source, CandidateSource::ParamEnv(ParamEnvSource::Global))); | ||
| if only_global_where_bounds { | ||
| Ok(self | ||
| .merge_candidates_or_bail_with_ambiguity(&candidates, TraitGoalProvenVia::ParamEnv)) |
There was a problem hiding this comment.
that !candidates.is_empty() is weird imo 🤔
do we just want to slap a single if candidates.is_empty() to the top of this function and then entirely rip out flounder?
There was a problem hiding this comment.
This makes sense, the only thing flounder added over bail_with_ambiguity was the empty check, so I moved that to the top of merge_trait_candidates as a single early return. With that, every branch now ends in bail_with_ambiguity.
6302baf to
0c27f4b
Compare
This branch has not been deployed
4 participants
closes: rust-lang/trait-system-refactor-initiative#35
r? lcnr