Error on projection of dyn noncompat type in old trait solver - #154992
Error on projection of dyn noncompat type in old trait solver#154992spirali wants to merge 3 commits into
Conversation
| //~^ ERROR the trait `Foo` is not dyn compatible | ||
| x.method(); | ||
| //~^ ERROR the trait `Foo` is not dyn compatible | ||
| //~^ ERROR no method named `method` found for reference `&dyn Foo` |
There was a problem hiding this comment.
that's annoying :< ideally we'd silence method errors if the self type is not well-formed, tracking that is non-trivial and I don't know how we'd do so myself rn
|
lgtm, let's do a crater run, there should be exactly 1 regression from this @bors try |
This comment has been minimized.
This comment has been minimized.
Error on projection of dyn noncompat type in old trait solver
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
Footnotes
|
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
@craterbot p=1 |
|
📝 Configuration of the ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
Footnotes
|
|
@craterbot check crates=https://crater-reports.s3.amazonaws.com/pr-154992-1/retry-regressed-list.txt p=1 surprising that we have that many spurious regressions still? it's caused by build OOMs and unlike the new solver, this PR shouldn't really change max-rss in any way 😅 cc @rust-lang/infra |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
Alright 🤔 @spirali can you look at the crater report and figure out whether they are expected? |
|
Oh, I had some problems with notifications again. I will look at it.
|
|
It seems that all fails are realated to a single crate: it fails with error: It seems that error message is correct as the |
|
yeah, valid regression, these error messages are horrible though 😅 I think getting errors at every callsite is hard to avoid rn. I don't quite get why we get type mismatch errors here though. I would hope that failing to normalize would instead just result in stuff being tainted or sth, silencing/avoidng the type mismatch do you have an idea what's going on there/are you able to minimize that? |
This comment has been minimized.
This comment has been minimized.
a17fd3c to
882e563
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
882e563 to
2ffefb5
Compare
|
I have implemented a new version. It suppresses downstream errors by returning a
|
This comment has been minimized.
This comment has been minimized.
|
|
||
| // `<dyn Trait>::Name` is only valid when `Trait` is dyn-compatible. | ||
| // If it isn't, create an error at the projection site and return a tainted error term. | ||
| let self_ty = selcx.infcx.shallow_resolve(obligation.predicate.self_ty()); |
There was a problem hiding this comment.
is there something preventing us from putting this into assemble_candidates_from_object_ty?
There was a problem hiding this comment.
AFAIK there is no easy way to produce an error term from assemble_candidates_from_object_ty. I wanted to avoid adding a new variant into ProjectionCandidateSet and then patching surrounding code.
But if you prefer it, I can add a new variant to ProjectionCandidateSet.
There was a problem hiding this comment.
hmm, could you manually call mark_error with https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/traits/enum.SelectionError.html#variant.TraitDynIncompatible instead of eagerly reporting an error there?
I think that should do what we want 🤔
There was a problem hiding this comment.
Calling mark_error (without eager reporting) will produce the following worse error message on git-url-parse-rs:
error[E0283]: type annotations needed
--> src/types/spec.rs:374:10
|
374 | ) -> impl Parser<
| __________^
375 | | &'url str,
376 | | Output = <dyn Parser<&'url str, Output = &'url str, Error = nom::error::Error<&'url str>> as Parser<
377 | | &'url str,
378 | | >>::Output,
379 | | Error = nom::error::Error<&'url str>,
380 | | >{
| |_____^ cannot infer type
...
387 | / context(
388 | | "Path parser (abempty)",
389 | | recognize(many1(pair(
390 | | tag("/"),
391 | | take_while(|c: char| pchar_uri_chars(c)),
392 | | ))),
393 | | )
| |_________- return type was inferred to be `nom::error::Context<impl Parser<_, Output = _, Error = _>>` here
|
= note: multiple `impl`s satisfying `_: nom::error::ParseError<&'url str>` found in the `nom` crate:
- impl<I> nom::error::ParseError<I> for ();
- impl<I> nom::error::ParseError<I> for (I, nom::error::ErrorKind);
- impl<I> nom::error::ParseError<I> for nom::error::Error<I>;
error[E0283]: type annotations needed
--> src/types/spec.rs:398:10
|
398 | ) -> impl Parser<
| __________^
399 | | &'url str,
400 | | Output = <dyn Parser<&'url str, Output = &'url str, Error = nom::error::Error<&'url str>> as Parser<
401 | | &'url str,
402 | | >>::Output,
403 | | Error = nom::error::Error<&'url str>,
404 | | >{
| |_____^ cannot infer type
...
410 | / context(
411 | | "Path parser (ssh)",
412 | | recognize((
413 | | tag(":"),
... |
416 | | )),
417 | | )
| |_________- return type was inferred to be `nom::error::Context<impl Parser<_, Output = _, Error = _>>` here
|
= note: multiple `impl`s satisfying `_: nom::error::ParseError<&'url str>` found in the `nom` crate:
- impl<I> nom::error::ParseError<I> for ();
- impl<I> nom::error::ParseError<I> for (I, nom::error::ErrorKind);
- impl<I> nom::error::ParseError<I> for nom::error::Error<I>;
error[E0283]: type annotations needed
--> src/types/spec.rs:422:10
|
422 | ) -> impl Parser<
| __________^
423 | | &'url str,
424 | | Output = <dyn Parser<&'url str, Output = &'url str, Error = nom::error::Error<&'url str>> as Parser<
425 | | &'url str,
426 | | >>::Output,
427 | | Error = nom::error::Error<&'url str>,
428 | | >{
| |_____^ cannot infer type
...
434 | / context(
435 | | "Path parser (rootless)",
436 | | recognize(pair(
437 | | take_while(|c: char| pchar_uri_chars(c)),
438 | | many0(pair(tag("/"), take_while(|c: char| pchar_uri_chars(c)))),
439 | | )),
440 | | )
| |_________- return type was inferred to be `nom::error::Context<impl Parser<_, Output = _, Error = _>>` here
|
= note: multiple `impl`s satisfying `_: nom::error::ParseError<&'url str>` found in the `nom` crate:
- impl<I> nom::error::ParseError<I> for ();
- impl<I> nom::error::ParseError<I> for (I, nom::error::ErrorKind);
- impl<I> nom::error::ParseError<I> for nom::error::Error<I>;
error[E0283]: type annotations needed
--> src/types/spec.rs:444:43
|
444 | fn short_git_scheme_parser<'url>() -> impl Parser<
| ___________________________________________^
445 | | &'url str,
446 | | Output = <dyn Parser<
447 | | &'url str,
... |
451 | | Error = nom::error::Error<&'url str>,
452 | | > {
| |_____^ cannot infer type
...
458 | / context(
459 | | "short git scheme parse",
460 | | opt(terminated(
461 | | tag::<&str, &str, nom::error::Error<&str>>("git"),
462 | | tag::<&str, &str, nom::error::Error<&str>>(":"),
463 | | )),
464 | | )
| |_________- return type was inferred to be `Context<impl Parser<&str, Output = Option<...>, Error = ...>>` here
|
= note: multiple `impl`s satisfying `_: nom::error::ParseError<&'url str>` found in the `nom` crate:
- impl<I> nom::error::ParseError<I> for ();
- impl<I> nom::error::ParseError<I> for (I, nom::error::ErrorKind);
- impl<I> nom::error::ParseError<I> for nom::error::Error<I>;
= note: the full name for the type has been written to '/home/ada/projects/git-url-parse-rs/target/markerror-test/debug/deps/git_url_parse-361fe5f41c7460ea.long-type-1968890314864789529.txt'
= note: consider using `--verbose` to print the full type name to the console
error[E0282]: type annotations needed
--> src/types/spec.rs:79:35
|
79 | return Ok((input, scheme.map(|s| s.to_string())));
| ^^^^^^ cannot infer type
error[E0282]: type annotations needed for `(&str, _)`
--> src/types/spec.rs:141:13
|
141 | let (input, path) = context(
| ^^^^^^^^^^^^^
...
157 | path: path.to_string(),
| ---- type must be known at this point
|
help: consider giving this pattern a type, where the placeholders `_` are specified
|
141 | let (input, path): (&_, Output) = context(
| ++++++++++++++
|
Very cool 😊 would you be open to write an types FCP proposal for this change? I'd gladly help out so reach out if you're unsure about what exactly to write/how to explain parts of it |
This comment has been minimized.
This comment has been minimized.
2ffefb5 to
b36a8bd
Compare
|
Some changes occurred in src/tools/cargo cc @weihanglo |
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
b36a8bd to
1c18ee4
Compare
FCPSummaryProjecting an associated type through The old solver did not check dyn compatibility when normalizing such a This is an instance of #100041: normalizing a The new solver always checks dyn compatibility when using an object candidate, Exampletrait Trait {
type Output;
fn process<T>(&mut self, input: T) -> T; // not dyn compatible
}
struct MyImpl;
impl Trait for MyImpl {
type Output = &'static str;
fn process<T>(&mut self, input: T) -> T { input }
}
fn make() -> impl Trait<Output = <dyn Trait<Output = &'static str> as Trait>::Output> {
MyImpl
}
fn main() {}Before this PR this compiles. After it: Crater reportCrater found a single affected crate, |
|
@rfcbot fcp merge types |
|
@lcnr has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
View all comments
Trying to fix rust-lang/trait-system-refactor-initiative#269
I am not sure that the check is placed into the right place in the code.
r? lcnr