Fix ICE when arg types can't be found in impl/trait methods while comparing#35877

KiChjang · 2016-08-21T20:16:14Z

Fixes #35869.

rust-highfive · 2016-08-21T20:16:26Z

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

KiChjang · 2016-08-21T20:17:51Z

r? @jonathandturner

KiChjang · 2016-08-21T21:54:00Z

Gah, this isn't as easy as I thought, we'd have to go through all wrapper types in the argument...

KiChjang · 2016-08-22T11:06:21Z

src/librustc_typeck/check/compare_method.rs

Hopefully I caught all the necessary types in this match...

nikomatsakis · 2016-08-22T15:52:12Z

r? @nikomatsakis

KiChjang · 2016-08-23T02:02:07Z

Okay, discussions with @eddyb on IRC indicated that this enormous match against types is pretty hacky and inaccurate. Instead, we should either:

Copy the FnSig Relate impl and recording where the mismatch happened; or
Have a second pass at sub_types for each argument in case of an error (the calling syntax would be infcx.sub_types(true, origin, trait_arg_ty, impl_arg_ty))

Both of which are much better than traversing through the type variants that I currently have in my implementation.

KiChjang · 2016-08-24T17:46:43Z

The giant match is gone, and is now replaced with several passes to infcx.sub_types for each argument. r? @nikomatsakis

eddyb · 2016-08-24T18:14:22Z

src/librustc_typeck/check/compare_method.rs

This should be last, because it's checked last in the whole function check.

KiChjang · 2016-08-24T18:49:17Z

Comments addressed.

arielb1 · 2016-08-25T20:40:09Z

@bors r+

bors · 2016-08-25T20:40:10Z

📌 Commit 3d9cf30 has been approved by arielb1

arielb1 · 2016-08-25T21:38:59Z

I think this would have an ugly span when the mismatch is on the return type. e.g. when the trait fn is fn foo(&self) -> i32 and the impl fn is fn foo(&self) -> i64.

@bors r-

KiChjang · 2016-08-26T00:30:30Z

src/librustc_typeck/check/compare_method.rs

+                        .unwrap_or_else(|| {
+                            if infcx.sub_types(false, origin, impl_sig.output,
+                                               trait_sig.output).is_err() {
+                                (impl_m_output.span(), Some(trait_m_output.span()))


@arielb1 I don't see a way to improve this? If we're not grabbing the span of the type, are we supposed to grab the span of the entire function declaration?

arielb1 · 2016-08-26T21:57:51Z

@bors r+

bors · 2016-08-26T21:57:52Z

📌 Commit 3d9cf30 has been approved by arielb1

nikomatsakis · 2016-08-26T22:36:43Z

src/test/compile-fail/issue-35869.rs

+
+impl Foo for Bar {
+    fn foo(_: fn(u16) -> ()) {}
+    //~^ ERROR method `foo` has an incompatible type for trait


Nit: maybe this should be a ui test? It's hard to tell just what is being highlighted here. :)

nikomatsakis · 2016-08-26T22:37:22Z

I've not had a chance to read this patch carefully, but if @arielb1 is satisfied, I guess I am. ;) Still, I think a ui test would be better.

Fix ICE when arg types can't be found in impl/trait methods while comparing Fixes rust-lang#35869.

bors · 2016-08-27T05:50:26Z

⌛ Testing commit 3d9cf30 with merge 698d4c9...

bors · 2016-08-27T06:51:26Z

⛄ The build was interrupted to prioritize another pull request.

bors · 2016-08-27T06:57:18Z

⌛ Testing commit 3d9cf30 with merge 6b74503...

Fix ICE when arg types can't be found in impl/trait methods while comparing Fixes #35869.

bors · 2016-08-27T09:49:19Z

Rollup of 7 pull requests - Successful merges: #35124, #35877, #35953, #36002, #36004, #36005, #36014 - Failed merges:

durka · 2016-08-30T22:18:32Z

Did this not fix the bug or is the playpen not updating anymore?

rust-highfive assigned pnkfelix Aug 21, 2016

rust-highfive assigned sophiajt and unassigned pnkfelix Aug 21, 2016

frewsxcv mentioned this pull request Aug 21, 2016

compiler panic using nightly on rustc_typeck::check::compare_method::compare_impl_method::_{{closure}}::h08bef90d287aed42 #35869

Closed

KiChjang force-pushed the issue-35869 branch from 3f3220d to 1bd3282 Compare August 21, 2016 21:51

KiChjang force-pushed the issue-35869 branch from 8d90754 to 41e3986 Compare August 22, 2016 09:15

KiChjang reviewed Aug 22, 2016
View reviewed changes

src/librustc_typeck/check/compare_method.rs Outdated

Copy link

Contributor Author

KiChjang Aug 22, 2016

Choose a reason for hiding this comment

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

Hopefully I caught all the necessary types in this match...

rust-highfive assigned nikomatsakis and unassigned sophiajt Aug 22, 2016

KiChjang force-pushed the issue-35869 branch from 41e3986 to b9ead66 Compare August 24, 2016 02:11

eddyb reviewed Aug 24, 2016
View reviewed changes

src/librustc_typeck/check/compare_method.rs Outdated

Copy link

Member

eddyb Aug 24, 2016

Choose a reason for hiding this comment

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

This should be last, because it's checked last in the whole function check.

KiChjang force-pushed the issue-35869 branch from b9ead66 to 5f5706d Compare August 24, 2016 18:14

KiChjang added 2 commits August 24, 2016 11:48

Extract error reporting into its own fn

07e6e81

More robust testing

3d9cf30

KiChjang force-pushed the issue-35869 branch from 5f5706d to 3d9cf30 Compare August 24, 2016 18:48

KiChjang reviewed Aug 26, 2016
View reviewed changes

nikomatsakis reviewed Aug 26, 2016
View reviewed changes

Manishearth added a commit to Manishearth/rust that referenced this pull request Aug 27, 2016

Rollup merge of rust-lang#35877 - KiChjang:issue-35869, r=arielb1

b5969ca

Fix ICE when arg types can't be found in impl/trait methods while comparing Fixes rust-lang#35869.

Manishearth mentioned this pull request Aug 27, 2016

Rollup of 7 pull requests #36030

Merged

bors added a commit that referenced this pull request Aug 27, 2016

Auto merge of #35877 - KiChjang:issue-35869, r=arielb1

6b74503

Fix ICE when arg types can't be found in impl/trait methods while comparing Fixes #35869.

bors merged commit 3d9cf30 into rust-lang:master Aug 27, 2016

bors added a commit that referenced this pull request Aug 27, 2016

Auto merge of #36030 - Manishearth:rollup, r=Manishearth

099b9fd

Rollup of 7 pull requests - Successful merges: #35124, #35877, #35953, #36002, #36004, #36005, #36014 - Failed merges:

KiChjang deleted the issue-35869 branch August 27, 2016 10:21

TimNN mentioned this pull request Aug 27, 2016

Impl check with trait ICE #36052

Closed

Stebalien mentioned this pull request Aug 27, 2016

Certain wrong code crashes nightly rustc #36051

Closed

Aatch mentioned this pull request Aug 28, 2016

ICE when using generics, associated types and necromantic copy/paste #36013

Closed

Uh oh!

Conversation

KiChjang commented Aug 21, 2016

Uh oh!

rust-highfive commented Aug 21, 2016

Uh oh!

KiChjang commented Aug 21, 2016

Uh oh!

KiChjang commented Aug 21, 2016

Uh oh!

KiChjang Aug 22, 2016

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Aug 22, 2016

Uh oh!

KiChjang commented Aug 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KiChjang commented Aug 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eddyb Aug 24, 2016

Choose a reason for hiding this comment

Uh oh!

KiChjang commented Aug 24, 2016

Uh oh!

arielb1 commented Aug 25, 2016

Uh oh!

bors commented Aug 25, 2016

Uh oh!

arielb1 commented Aug 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KiChjang Aug 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1 commented Aug 26, 2016

Uh oh!

bors commented Aug 26, 2016

Uh oh!

nikomatsakis Aug 26, 2016

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Aug 26, 2016

Uh oh!

bors commented Aug 27, 2016

Uh oh!

bors commented Aug 27, 2016

Uh oh!

bors commented Aug 27, 2016

Uh oh!

bors commented Aug 27, 2016

Uh oh!

durka commented Aug 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

KiChjang commented Aug 23, 2016 •

edited

Loading

KiChjang commented Aug 24, 2016 •

edited

Loading

arielb1 commented Aug 25, 2016 •

edited

Loading

KiChjang Aug 26, 2016 •

edited

Loading