Cast suggestions by GuillaumeGomez · Pull Request #38099 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation32 Commits22 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

self.fcx.associated_item(def_id, self.item_name)
match self.looking_for {
LookingFor::MethodName(item_name) => self.fcx.associated_item(def_id, item_name),
_ => None,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikomatsakis: This matching is invalid, but I don't know what to put here. We don't have a method name to refer to, just a return type. Won't it always return the same method if I match on it?

@eddyb: Maybe you have an idea here?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this method should go away. Callers can just use impl_or_trait_item instead. I've got a local branch doing that (might also try modifying impl_or_trait_item to return impl Iterator, since right now it only considers the first method that matches the return type...?)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. You're right about associated_item being wrong now though.

@@ -1364,9 +1364,9 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
cause: &ObligationCause<'tcx>,
expected: Ty<'tcx>,
actual: Ty<'tcx>,
err: TypeError<'tcx>) {
err: TypeError<'tcx>) -> DiagnosticBuilder<'tcx> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: newline before -> here

fn format_method_suggestion(&self, method: &AssociatedItem) -> String {
format!(".{}({})",
method.name,
if self.has_not_input_arg(method) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not: has_no_input_arg or does_not_have_input_arg or !self.has_input_arg :)

self.fcx.associated_item(def_id, self.item_name)
match self.looking_for {
LookingFor::MethodName(item_name) => self.fcx.associated_item(def_id, item_name),
_ => None,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this method should go away. Callers can just use impl_or_trait_item instead. I've got a local branch doing that (might also try modifying impl_or_trait_item to return impl Iterator, since right now it only considers the first method that matches the return type...?)

\|
= note: expected type `&mut std:🧵:String`
= note: found type `&std:🧵:String`
= help: try with `&mut y`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these changes that suggest a & still in the PR?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't test since the current code was wrong. Once updated, this will follow.

OK, so, I pushed a quick fix, but we still fail 3 compile-fail tests. I'm not 100% sure why, didn't dig into it yet:

    [compile-fail] compile-fail/issue-5500.rs
    [compile-fail] compile-fail/occurs-check-2.rs
    [compile-fail] compile-fail/occurs-check.rs

each of them fails with a rather useless "error: the type of this value must be known in this context".

@nikomatsakis: We have an issue. With the current code, it only returns one proposition. For example for the following code:

fn foo(_x: String) {}

fn main() { foo("a"); }

It proposes only replace method while other methods are also available. This is a huge step back, at least the first versions were able to propose multiple propositions. :-/

We have an issue. With the current code, it only returns one proposition. For example for the following code:

I think that is because the impl_or_trait_item method returns Option and not Vec (or impl Iterator). Should be easily fixed, but I didn't do it yesterday because I was more concerned about the type inference regressions, which seemed worth fixing.

Indeed. I'll give it a try then.

Ok, so I fixed this issue. Code is a bit ugly unfortunately. We still have the same failure for the 3 tests.

EDIT: Oh also, suggestions are completely stupid haha.

☔ The latest upstream changes (presumably #38079) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez I pushed a commit that cleans up the code a bit, but I still see various failures around inference that I think we need to track down:

failures:
    [compile-fail] compile-fail/issue-5500.rs
    [compile-fail] compile-fail/occurs-check-2.rs
    [compile-fail] compile-fail/occurs-check.rs

OK, I see those errors were the cause of your changes to not call unambiguous_final_ty. I've pushed a commit with an alternative approach there that I think will fix those test cases.

BTW, did you have some suggestion tests that were failing because of the fact that we were returning an Option from impl_or_trait_item() instead of a Vec?

I ran make check-stage3 and didn't have other failures. So I guess we're good?

I fixed the linter as well.

@nikomatsakis: If you're referring to travis' failure, it seems normal. If not, then what are you referring to?

@GuillaumeGomez

I see this, is that normal?

error: attempted to take value of method `inputs` on type `&rustc::ty::FnSig<'_>`

   --> /checkout/src/librustc_typeck/check/demand.rs:118:47

    |

118 |                         fty.sig.skip_binder().inputs.len() == 1

    |                                               ^^^^^^

    |

    = help: maybe a `()` to call it is missing? If not, try an anonymous function

I guess that particular configuration does seem to typically have a travis failure of some kind.

I think that error might be from #38097, where inputs became a function on FnSig. I assume this was either somehow missed in that, or added in this PR and as such not covered.

Uh?! I guess I failed my update and my rebuild... Taking another look then.

So as I thought, I needed to rebase once again. Now it's fixed and tests are passing again.

@GuillaumeGomez sadly it seems like travis is still unhappy! The coerce-suggestions test still seems to be failing :(

Some of the output:

diff of stderr:

 20 - |   = help: try with `&String::new()`|
    + |   = help: here are some functions which might fulfill your needs:|
 21 - ||
    + | - .as_str()|
...

When we are scanning for suggestions, an unresolved inference variable is not a hard error.

And travis tests passed! \o/

📌 Commit 28e2c6a has been approved by nikomatsakis

⌛ Testing commit 28e2c6a with merge 439c312...

bors added a commit that referenced this pull request

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild
Approved by: nikomatsakis
Pushing 439c312 to master...

This was referenced

Dec 21, 2016

est31 mentioned this pull request

est31 added a commit to est31/rust that referenced this pull request

This removes the safe_suggestion feature from feature_gate.rs. It was added in commit 164f010 and then removed again in commit c11fe55 .

As the removal was in the same PR rust-lang#38099 as the addition, we don't move it to the "removed" section.

Removes an element from the whitelist of non gate tested unstable lang features (issue rust-lang#39059).

bors added a commit that referenced this pull request

Mark safe_suggestion and pushpop_unsafe as removed in feature_gate.rs

This removes two features from feature_gate.rs: safe_suggestion and pushpop_unsafe. Both had been removed in other places already, but were forgotten to be removed from feature_gate.rs.

safe_suggestion was added in commit 164f010 and then removed again in commit c11fe55 both in the same PR #38099.
pushpop_unsafe was added in commit 1829fa5 and removed again in commit d399098

Removes two elements from the whitelist of non gate tested unstable lang features (issue #39059).

bors added a commit that referenced this pull request

Mark safe_suggestion and pushpop_unsafe as removed in feature_gate.rs

This removes two features from feature_gate.rs: safe_suggestion and pushpop_unsafe. Both had been removed in other places already, but were forgotten to be removed from feature_gate.rs.

safe_suggestion was added in commit 164f010 and then removed again in commit c11fe55 both in the same PR #38099.
pushpop_unsafe was added in commit 1829fa5 and removed again in commit d399098

Removes two elements from the whitelist of non gate tested unstable lang features (issue #39059).