Cast suggestions #38099

GuillaumeGomez · 2016-12-01T01:02:59Z

GuillaumeGomez · 2016-12-01T06:24:59Z

src/librustc_typeck/check/method/probe.rs

-        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,


@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?

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...?)

nikomatsakis

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

nikomatsakis · 2016-12-01T20:16:14Z

src/librustc/infer/mod.rs

@@ -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> {


Nit: newline before -> here

nikomatsakis · 2016-12-01T20:16:48Z

src/librustc_typeck/check/demand.rs

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


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

nikomatsakis · 2016-12-01T20:25:40Z

src/librustc_typeck/check/method/probe.rs

-        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,


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...?)

nikomatsakis · 2016-12-01T20:26:18Z

src/test/ui/span/coerce-suggestions.stderr

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


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

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

nikomatsakis · 2016-12-01T20:52:35Z

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".

GuillaumeGomez · 2016-12-02T02:30:23Z

@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. :-/

nikomatsakis · 2016-12-02T19:01:09Z

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.

GuillaumeGomez · 2016-12-02T22:09:00Z

Indeed. I'll give it a try then.

GuillaumeGomez · 2016-12-03T00:45:22Z

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.

bors · 2016-12-03T21:00:32Z

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

nikomatsakis · 2016-12-09T15:24:02Z

@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

nikomatsakis · 2016-12-09T15:42:12Z

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?

GuillaumeGomez · 2016-12-09T17:38:30Z

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

GuillaumeGomez · 2016-12-09T17:42:34Z

I fixed the linter as well.

nikomatsakis · 2016-12-09T19:25:51Z

@GuillaumeGomez still some kind of issue...

GuillaumeGomez · 2016-12-10T13:12:46Z

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

nikomatsakis · 2016-12-12T17:27:22Z

@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

nikomatsakis · 2016-12-12T17:28:13Z

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

Mark-Simulacrum · 2016-12-12T20:20:09Z

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.

GuillaumeGomez · 2016-12-12T20:32:24Z

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

GuillaumeGomez · 2016-12-13T01:22:02Z

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

nikomatsakis · 2016-12-19T20:47:57Z

@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.

GuillaumeGomez · 2016-12-20T10:44:02Z

@nikomatsakis: Indeed, didn't see this one. Should be fixed now.

GuillaumeGomez · 2016-12-20T14:07:52Z

And travis tests passed! \o/

GuillaumeGomez · 2016-12-20T19:34:03Z

@nikomatsakis: no r+?

nikomatsakis · 2016-12-20T20:28:01Z

@bors r+

bors · 2016-12-20T20:28:01Z

📌 Commit 28e2c6a has been approved by nikomatsakis

bors · 2016-12-21T07:28:14Z

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

@nikomatsakis

Cast suggestions r? @nikomatsakis

bors · 2016-12-21T10:38:09Z

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).

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).

rust-highfive assigned nikomatsakis Dec 1, 2016

GuillaumeGomez force-pushed the cast_suggestions branch from 5c5e1e8 to 3fb3239 Compare December 1, 2016 01:50

GuillaumeGomez commented Dec 1, 2016

View reviewed changes

nikomatsakis requested changes Dec 1, 2016

View reviewed changes

GuillaumeGomez force-pushed the cast_suggestions branch from df0ebff to 0b433e9 Compare December 4, 2016 07:25

nikomatsakis force-pushed the cast_suggestions branch from b7dc50f to 05d1958 Compare December 9, 2016 16:06

GuillaumeGomez force-pushed the cast_suggestions branch from 5816ccc to e2c5370 Compare December 13, 2016 01:21

GuillaumeGomez added 5 commits December 20, 2016 11:37

Start of implementation of proposal for E0308

601bbf2

Add safe_suggestion attribute

164f010

Move suggestion list creation to coerce check

021d97d

Return DiagnosticBuilder to add help suggestions

07a3ede

Move matches return type method

8057d48

nikomatsakis and others added 6 commits December 20, 2016 11:37

fix some broken tests

dac8883

Allow multiple suggestions

25a2d13

don't special case having 1 item

5d41be3

recover from unresolved inference variable at end of autoderef

6ea1fbb

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

Fix linter issue

f439a24

Update ui test stderr

28e2c6a

GuillaumeGomez force-pushed the cast_suggestions branch from e2c5370 to 28e2c6a Compare December 20, 2016 10:43

nikomatsakis approved these changes Dec 20, 2016

View reviewed changes

bors added a commit that referenced this pull request Dec 21, 2016

Auto merge of #38099 - GuillaumeGomez:cast_suggestions, r=nikomatsakis

439c312

Cast suggestions r? @nikomatsakis

bors merged commit 28e2c6a into rust-lang:master Dec 21, 2016

This was referenced Dec 21, 2016

Fix associated types in copy implementations #38152

Merged

Ref suggestion #37658

Merged

i128 and u128 support #38482

Merged

calling convention for MSP430 interrupts #38465

Merged

Implement kind="static-nobundle" (RFC 1717) #38426

Merged

SimonSapin mentioned this pull request Dec 22, 2016

Type inference regression #38535

Closed

est31 mentioned this pull request Jan 14, 2017

Mark safe_suggestion and pushpop_unsafe as removed in feature_gate.rs #39071

Merged

GuillaumeGomez deleted the cast_suggestions branch November 24, 2017 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cast suggestions #38099

Cast suggestions #38099

GuillaumeGomez commented Dec 1, 2016

GuillaumeGomez Dec 1, 2016

nikomatsakis Dec 1, 2016

nikomatsakis left a comment

nikomatsakis Dec 1, 2016

nikomatsakis Dec 1, 2016

nikomatsakis Dec 1, 2016

nikomatsakis Dec 1, 2016

GuillaumeGomez Dec 1, 2016

nikomatsakis commented Dec 1, 2016

GuillaumeGomez commented Dec 2, 2016

nikomatsakis commented Dec 2, 2016

GuillaumeGomez commented Dec 2, 2016

GuillaumeGomez commented Dec 3, 2016 •

edited

Loading

bors commented Dec 3, 2016

nikomatsakis commented Dec 9, 2016

nikomatsakis commented Dec 9, 2016

GuillaumeGomez commented Dec 9, 2016

GuillaumeGomez commented Dec 9, 2016

nikomatsakis commented Dec 9, 2016

GuillaumeGomez commented Dec 10, 2016

nikomatsakis commented Dec 12, 2016

nikomatsakis commented Dec 12, 2016

Mark-Simulacrum commented Dec 12, 2016

GuillaumeGomez commented Dec 12, 2016

GuillaumeGomez commented Dec 13, 2016 •

edited

Loading

nikomatsakis commented Dec 19, 2016

GuillaumeGomez commented Dec 20, 2016

GuillaumeGomez commented Dec 20, 2016

GuillaumeGomez commented Dec 20, 2016

nikomatsakis commented Dec 20, 2016

bors commented Dec 20, 2016

bors commented Dec 21, 2016

bors commented Dec 21, 2016

Cast suggestions #38099

Cast suggestions #38099

Conversation

GuillaumeGomez commented Dec 1, 2016

GuillaumeGomez Dec 1, 2016

Choose a reason for hiding this comment

nikomatsakis Dec 1, 2016

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Dec 1, 2016

Choose a reason for hiding this comment

nikomatsakis Dec 1, 2016

Choose a reason for hiding this comment

nikomatsakis Dec 1, 2016

Choose a reason for hiding this comment

nikomatsakis Dec 1, 2016

Choose a reason for hiding this comment

GuillaumeGomez Dec 1, 2016

Choose a reason for hiding this comment

nikomatsakis commented Dec 1, 2016

GuillaumeGomez commented Dec 2, 2016

nikomatsakis commented Dec 2, 2016

GuillaumeGomez commented Dec 2, 2016

GuillaumeGomez commented Dec 3, 2016 • edited Loading

bors commented Dec 3, 2016

nikomatsakis commented Dec 9, 2016

nikomatsakis commented Dec 9, 2016

GuillaumeGomez commented Dec 9, 2016

GuillaumeGomez commented Dec 9, 2016

nikomatsakis commented Dec 9, 2016

GuillaumeGomez commented Dec 10, 2016

nikomatsakis commented Dec 12, 2016

nikomatsakis commented Dec 12, 2016

Mark-Simulacrum commented Dec 12, 2016

GuillaumeGomez commented Dec 12, 2016

GuillaumeGomez commented Dec 13, 2016 • edited Loading

nikomatsakis commented Dec 19, 2016

GuillaumeGomez commented Dec 20, 2016

GuillaumeGomez commented Dec 20, 2016

GuillaumeGomez commented Dec 20, 2016

nikomatsakis commented Dec 20, 2016

bors commented Dec 20, 2016

bors commented Dec 21, 2016

bors commented Dec 21, 2016

GuillaumeGomez commented Dec 3, 2016 •

edited

Loading

GuillaumeGomez commented Dec 13, 2016 •

edited

Loading