-
Notifications
You must be signed in to change notification settings - Fork 12.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rust beta: "cannot infer type" when compiling bottom
crate
#96074
Comments
This is almost certainly caused by #95098. cc @shepmaster A smaller repro: #[derive(Clone)]
struct Constraint;
fn constraints<C>(constraints: C)
where C: Into<Vec<Constraint>>
{
let _: Vec<Constraint> = constraints.into();
}
fn main() {
constraints(vec![Constraint].as_ref());
} |
Assigning priority as discussed in the Zulip thread of the Prioritization Working Group. @rustbot label -I-prioritize +P-high |
For T-libs-api: I don't expect that we would choose to revert this. @rust-lang/libs-api However it's not clear to me what type inference limitation makes the type unable to be inferred. Using the minimal repro from @ehuss, we have: constraints(vec.as_ref()); Rustc can see that The possibilities for Then it sees calling Is it computationally hard to deduce from this that |
I recall some special cases in candidate assembly for allowing inference to continue when there is only one unambiguous impl, if I had to guess I'd say this is the same issue as #90904. I'm trying to find the special case, I think this might be it but I'm not sure: rust/compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs Lines 140 to 156 in b04c532
Sadly I don't understand this well enough either to really understand whats going on. I think there's some rule in the compiler (maybe the bit above) that lets the ambiguous type parameter for |
Perhaps copy-paste from the tui crate's docs: let chunks = Layout::default()
.direction(Direction::Vertical)
.margin(1)
.constraints(
[
Constraint::Percentage(10),
Constraint::Percentage(80),
Constraint::Percentage(10)
].as_ref()
)
.split(f.size());
pub fn constraints<C>(mut self, constraints: C) -> Layout
where
C: Into<Vec<Constraint>>, |
My original justification was not uniformity, but to allow passing byte strings to Amusingly, the TUI code that fails is improved when the code is changed to avoid the new error (IMO): // doesn't compile
.constraints(vec![Constraint::Percentage(10)].as_ref())
// compiles
.constraints(vec![Constraint::Percentage(10)].as_slice())
.constraints(vec![Constraint::Percentage(10)])
.constraints([Constraint::Percentage(10)].as_ref())
.constraints(&[Constraint::Percentage(10)])
.constraints([Constraint::Percentage(10)]) |
If we think we can improve inference in the compiler to avoid this, then this does seem worth reverting on beta. cc @rust-lang/wg-traits @rust-lang/compiler; does this seem fixable through better inference? |
The only problem I see is that changing inference to get these to work might be doable, but not necessarily in the right way, which scares me because it would necessarily mean having a more lenient version of I'd still be on favor of delaying landing this until 1.62 to buy us time to evaluate (but t-libs-api has jurisdiction). |
so my understanding is: we have two obligations we still have to prove:
the core issue here is that both obligations by themselves are rightfully ambiguous and as long as we continue to try and prove obligations by themselves breakage like this is unavoidable. A possible solution would be:
While something like that probably? works I don't think this is something we want. It would add a noticeable amount of complexity to our type inference and will end up causing inference to make some pretty large leaps in places where it is probably not expected. alternatively we somehow add some preference in the case of ambiguity, but no preference is always correct here. if the preference is incorrect the resulting error will be pretty bad. it will be a worse version of the existing errors mentioning |
👍 for revert and re-evaluate, @shepmaster correct me if I'm wrong but there's no urgency here for landing this asap, correct? I do think this might fall within the set of changes that we'd potentially just allow breakage on, similar to #75180 where we manually updated all the crates identified by crater. Additionally, I'm wondering if rust-lang/rfcs#3240 could be extended to cover this case so we could move the breakage to an edition boundary. I'm hoping that might allow for a simpler solution. |
Looking at Edit:
I was gonna make a point about how the inference resolver should strive to be as repeatable and clean as possible (while at the same time me thinking that UX trumps theoretical purity), but then I find this, and realize that's already gone out the window 😅 rust/compiler/rustc_middle/src/ty/mod.rs Lines 2099 to 2106 in 9861bc8
rust/compiler/rustc_ty_utils/src/ty.rs Lines 366 to 412 in db03a2d
|
(fair warning, I've only skimmed this thread) Interestingly, I think this might be a similar "problem" to the one that gets discussed with Chalk with the SLG vs recursive solver (some background: https://rust-lang.github.io/chalk/book/recursive.html#recursion-and-completeness). Without going into specific details, I think committing to a "smarter" inference here has practical effects in an eventual more-formal solver. I wouldn't want to apply an ad-hoc solution to this problem to the trait solver without thinking about how we implement this in the future. Also given that there are ongoing efforts to define that, I think reverting for now is a good option. |
None that I know of. The only downside is that I used the original PR as a way to show someone "look how easy it is to submit changes to Rust" 😉 |
…e without reverting rust-lang#95098 rust-lang#95098 introduces new `From` impls for `Vec`, which can break existing code that relies on inference when calling `.as_ref()`. This change explicitly carves out a bias in the inference machinery to keep existing code compiling, while still maintaining the new `From` impls. Reported in rust-lang#96074.
I came up with the following unprincipled hack: #96446 Doing anything like this would in effect turn both |
We discussed this in today's @rust-lang/libs meeting. Could we get a revert PR, please? We can then mark that as beta-nominated/beta-accepted. |
Opened #96489 |
For context, see: - #708 - rust-lang/rust#96074 This changes the calls from `as_ref()`, which was causing problems, to just `as_slice()` which works fine.
For context, see: - #708 - rust-lang/rust#96074 This changes the calls from `as_ref()`, which was causing type inference issues, to just `as_slice()` which works fine.
For context, see: - #708 - rust-lang/rust#96074 This changes the calls from `as_ref()`, which was causing type inference issues, to just `as_slice()` which works fine.
For context, see: - #708 - rust-lang/rust#96074 This changes the calls from `as_ref()`, which was causing type inference issues, to just `as_slice()` which works fine.
For context, see: - #708 - rust-lang/rust#96074 This changes the calls from `as_ref()`, which was causing type inference issues, to just `as_slice()` which works fine.
This changes various as_ref() calls as needed in order for bottom to successfully build in Rust beta 1.61, as they were causing type inference issues. These calls were either removed or changed to an alternative that does build (e.g. as_slice()). Functionally, there should be no change. For context, see: - #708 - rust-lang/rust#96074
I think making inference even 'smarter' is just going to lead to more code that implicitly relies on certain impls not existing, which will ultimately prohibit more impls from being added because some ambiguities just cannot be resolved cleanly. I would much rather see existing code that relies on there's-only-one-impl-for cleverness start raising future-incompat errors. Maybe we could even have |
:) @rustbot label -I-compiler-nominated |
Clearing milestone as we have landed #96489 on master (1.62+) and backported it to 1.61, so this is no longer a regression. |
Triage: The revert is done and consensus seems to be to not change inference. Closing. |
Not sure who is correct here, or if this is expected: I just compiled the bottom crate version 0.6.8. This worked with the stable compiler, but failed with the current beta. The error message is the following:
Meta
rustc --version --verbose
:The text was updated successfully, but these errors were encountered: