fix for ICE issue #25180 #25209

llogiq · 2015-05-08T11:41:30Z

According to @eddyb – and my tests – the following gets rid of the ICE in issue #25180.

rust-highfive · 2015-05-08T11:41:45Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

llogiq · 2015-05-08T13:09:59Z

I think what the PR lacks as of yet is a testcase to show that the fix actually quells the ICE. I just added it and will commit after I rebuild.

eddyb · 2015-05-08T14:09:47Z

src/librustc/middle/check_const.rs

-        assert!(self.mode == Mode::Var);
+        let (old_mode, old_qualif) = (self.mode, self.qualif);
+        self.mode = Mode::Var;
+        self.qualif = ConstQualif::empty();


You don't have to copy with_mode, you can just call it (and replace self in the two original statements with v, or whatever you choose for the name of the closure argument).

llogiq · 2015-05-08T14:19:20Z

Yep, you're right. I'll change it. Thank you for pointing that out.

… removed the with_mode copy, thanks to eddyb

eddyb · 2015-05-09T21:42:25Z

src/librustc/middle/check_const.rs

-        assert!(self.mode == Mode::Var);
-        self.with_euv(Some(fn_id), |euv| euv.walk_fn(fd, b));
+        self.with_mode(Mode::Var, |v| v.with_euv(Some(fn_id),
+            |euv| euv.walk_fn(fd, b)));
        visit::walk_fn(self, fk, fd, b, s);


walk_fn has to be inside the with_mode call, too.

llogiq · 2015-05-10T02:53:14Z

OK, but I'd like to understand why? Surely rustc compiles the code anyway – I even have a test case saying so. What are the implications of walking the fn in const mode?

eddyb · 2015-05-10T05:58:02Z

If it's in const mode, you get the const rules. And maybe ICEs. Try calling a function and you'll see.

llogiq · 2015-05-10T08:01:31Z

Ah, Ok. Should have run the extended test before I fixed the function.

Is this ready to merge now or do I need to do something else?

eddyb · 2015-05-10T08:18:00Z

src/test/run-pass/issue-25180.rs

+
+// pretty-expanded FIXME #25180
+
+const x: &'static Fn() = &|| println!("ICE here");


Do multiple statements, arguments and local variables work in these closures?

llogiq · 2015-05-10T08:19:04Z

Yeah, it's a good idea to add some more tests. I'm on it. Edit: Just started make check.

llogiq · 2015-05-10T10:02:28Z

So yes, it appears to work after I got the test code right.

eddyb · 2015-05-10T21:13:33Z

r? @eddyb @bors r+ rollup

bors · 2015-05-10T21:13:34Z

📌 Commit a18ce4d has been approved by eddyb

@eddyb

According to @eddyb – and my tests – the following gets rid of the ICE in issue rust-lang#25180.

@eddyb

According to @eddyb – and my tests – the following gets rid of the ICE in issue rust-lang#25180.

bors · 2015-05-11T08:37:04Z

⌛ Testing commit a18ce4d with merge b758a37...

@eddyb

According to @eddyb – and my tests – the following gets rid of the ICE in issue #25180.

bors · 2015-05-11T09:50:35Z

💔 Test failed - auto-mac-64-opt

eddyb · 2015-05-11T11:45:29Z

src/test/run-pass/issue-25180.rs

+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// pretty-expanded FIXME #25180


The failure was in pretty tests, which I don't you can do if you're using macros like println! which use unstable items in their expansions.

@eddyb

According to @eddyb – and my tests – the following gets rid of the ICE in issue rust-lang#25180.

- Successful merges: #25209, #25254, #25272, #25278, #25282, #25283, #25288, #25292, #25302, #25304 - Failed merges:

steveklabnik · 2015-05-12T03:24:05Z

@bors: rollup-

eddyb · 2015-05-12T04:12:38Z

@steveklabnik huh, shouldn't a test failure take it out of the queue automatically?

steveklabnik · 2015-05-12T04:28:40Z

don't think so

llogiq · 2015-05-12T17:39:40Z

@steveklabnik I'm sorry that I broke the rollup. Apparently my tests weren't sufficient. I've now removed the print! macro invocations and added a feature declaration (which was hinted in the error message).

Can someone tell me how I can reproduce the failing test?

steveklabnik · 2015-05-12T17:59:47Z

It's all good :) everyone does it. I'm not 100% sure how to reproduce though

llogiq · 2015-05-12T20:24:14Z

Thanks for the kind words. My first PR and manage to hit all available roadblocks at once.

But hey, I learn a lot there days. 😄

eddyb · 2015-05-12T20:36:32Z

src/test/run-pass/issue-25180.rs

+
+const EMPTY: &'static Fn() = &|| ();
+
+const ONE_ARGUMENT: &'static Fn(u32) = &|y| assert!(y == 42);


assert! still uses the formatting machinery. I'm curious if we prefer to disable pretty testing or just add feature gates. What do other tests do?

llogiq · 2015-05-13T08:44:03Z

I see 783 instances of "feature" just in test/run-pass. So yes, it appears feature-gates are not that uncommon. I'm unsure if #![feature(std_misc)] will be enough to make it work.

eddyb · 2015-05-13T08:59:33Z

@llogiq Running make check-stage1-rpass-pretty should answer that question.

llogiq · 2015-05-13T14:03:16Z

@eddyb Thanks, but that only prints that my Makefile does not have this target. Perhaps I'm missing some required package?

pnkfelix · 2015-05-13T14:09:02Z

@llogiq no I think @eddyb just mispoke. The target is (probably) make check-stage1-pretty-rpass.

(I always recommend GNU remake for debugging big makefile systems.)

eddyb · 2015-05-13T14:31:43Z

@llogiq @pnkfelix My bad, I have both in my shell history and I pasted the bad one.

llogiq · 2015-05-13T16:37:56Z

In that case, my previous two commits make the test pass on my machine. On the other hand, it doesn't even require the feature gate. So it appears I cannot reproduce the issue on my 64-bit intel / Linux machine (Though I'm not sure if this is really about darwin or if I missed something while building).

Can someone with a mac reproduce the problem?

bors · 2015-05-24T12:50:44Z

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

llogiq · 2015-06-04T16:23:35Z

I've just revisited the problem, and recent changes not only made my PR unmergeable, but also fixed the problem in a different way. Closing the PR is the sensible option here.

fixed ICE issue #25180

96229e8

rust-highfive assigned pnkfelix May 8, 2015

formatting correction

29daa29

eddyb reviewed May 8, 2015
View reviewed changes

crossed the i-s and dotted the t-s ;-) Also added a run-pass test and…

1434715

… removed the with_mode copy, thanks to eddyb

eddyb reviewed May 9, 2015
View reviewed changes

Pulled walk_fn() into Var mode, extended test

ca5c694

eddyb reviewed May 10, 2015
View reviewed changes

more tests

6e19cfa

fixed errors in test code

a18ce4d

rust-highfive assigned eddyb and unassigned pnkfelix May 10, 2015

steveklabnik added a commit to steveklabnik/rust that referenced this pull request May 10, 2015

Rollup merge of rust-lang#25209 - llogiq:master, r=eddyb

82f76a3

According to @eddyb – and my tests – the following gets rid of the ICE in issue rust-lang#25180.

steveklabnik mentioned this pull request May 10, 2015

Rollup of 4 pull requests #25285

Closed

Manishearth added a commit to Manishearth/rust that referenced this pull request May 11, 2015

Rollup merge of rust-lang#25209 - llogiq:master, r=eddyb

c40f6b9

According to @eddyb – and my tests – the following gets rid of the ICE in issue rust-lang#25180.

Manishearth mentioned this pull request May 11, 2015

Rollup of 9 pull requests #25298

Closed

bors added a commit that referenced this pull request May 11, 2015

Auto merge of #25209 - llogiq:master, r=eddyb

b758a37

According to @eddyb – and my tests – the following gets rid of the ICE in issue #25180.

eddyb reviewed May 11, 2015
View reviewed changes

steveklabnik added a commit to steveklabnik/rust that referenced this pull request May 11, 2015

Rollup merge of rust-lang#25209 - llogiq:master, r=eddyb

da75de8

According to @eddyb – and my tests – the following gets rid of the ICE in issue rust-lang#25180.

steveklabnik mentioned this pull request May 11, 2015

Rollup of 10 pull requests #25315

Closed

bors added a commit that referenced this pull request May 11, 2015

Auto merge of #25315 - steveklabnik:rollup, r=steveklabnik

1227d5d

- Successful merges: #25209, #25254, #25272, #25278, #25282, #25283, #25288, #25292, #25302, #25304 - Failed merges:

llogiq added 2 commits May 12, 2015 10:19

Removed println! from test

4eec41a

added feature declaration

3715898

eddyb reviewed May 12, 2015
View reviewed changes

llogiq closed this Jun 4, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix for ICE issue #25180 #25209

fix for ICE issue #25180 #25209

llogiq commented May 8, 2015

rust-highfive commented May 8, 2015

llogiq commented May 8, 2015

eddyb May 8, 2015

llogiq commented May 8, 2015

eddyb May 9, 2015

llogiq commented May 10, 2015

eddyb commented May 10, 2015

llogiq commented May 10, 2015

eddyb May 10, 2015

llogiq commented May 10, 2015

llogiq commented May 10, 2015

eddyb commented May 10, 2015

bors commented May 10, 2015

bors commented May 11, 2015

bors commented May 11, 2015

eddyb May 11, 2015

steveklabnik commented May 12, 2015

eddyb commented May 12, 2015

steveklabnik commented May 12, 2015

llogiq commented May 12, 2015

steveklabnik commented May 12, 2015

llogiq commented May 12, 2015

eddyb May 12, 2015

llogiq commented May 13, 2015

eddyb commented May 13, 2015

llogiq commented May 13, 2015

pnkfelix commented May 13, 2015

eddyb commented May 13, 2015

llogiq commented May 13, 2015

bors commented May 24, 2015

llogiq commented Jun 4, 2015


		// pretty-expanded FIXME #25180

		const x: &'static Fn() = &\|\| println!("ICE here");


		const EMPTY: &'static Fn() = &\|\| ();

		const ONE_ARGUMENT: &'static Fn(u32) = &\|y\| assert!(y == 42);

fix for ICE issue #25180 #25209

fix for ICE issue #25180 #25209

Conversation

llogiq commented May 8, 2015

rust-highfive commented May 8, 2015

llogiq commented May 8, 2015

eddyb May 8, 2015

Choose a reason for hiding this comment

llogiq commented May 8, 2015

eddyb May 9, 2015

Choose a reason for hiding this comment

llogiq commented May 10, 2015

eddyb commented May 10, 2015

llogiq commented May 10, 2015

eddyb May 10, 2015

Choose a reason for hiding this comment

llogiq commented May 10, 2015

llogiq commented May 10, 2015

eddyb commented May 10, 2015

bors commented May 10, 2015

bors commented May 11, 2015

bors commented May 11, 2015

eddyb May 11, 2015

Choose a reason for hiding this comment

steveklabnik commented May 12, 2015

eddyb commented May 12, 2015

steveklabnik commented May 12, 2015

llogiq commented May 12, 2015

steveklabnik commented May 12, 2015

llogiq commented May 12, 2015

eddyb May 12, 2015

Choose a reason for hiding this comment

llogiq commented May 13, 2015

eddyb commented May 13, 2015

llogiq commented May 13, 2015

pnkfelix commented May 13, 2015

eddyb commented May 13, 2015

llogiq commented May 13, 2015

bors commented May 24, 2015

llogiq commented Jun 4, 2015