Fix ICE per #28172

[alexispurslane]

This fixes the ICE, and makes it just a compiler error/warning. I'm not exactly sure that's whats wanted, so tell me if it isn't.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (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 the contribution instructions for more information.

[arielb1]

@ChristopherDumas

This leads to error triplication - one from global_expr, one from const_eval and the one you added. I would const_eval the sides, error-return if this failed, compare them, and then global_expr (the global_expr is needed to set up the flags).

Also, you need to add a test (copy one of the files in src/test/compile-fail/ and edit it).

[alexispurslane]

@arielb1 Can you elaborate a little more? I'm new to Rust, so this doesn't make much sense to me.

[arielb1]

@ChristopherDumas

First, we add a test for every bug fixed in rustc. The tests we add are typically compiletests - these are .rs files which are compiled and the compilation result is checked. Because #28172 is erroneous code, your test will check that it is rejected with a correct error message - this is called a compile-fail test. All files in src/test/compile-fail are compile-fail tests.

// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

fn main() {
    let x = 0;
    match 1 {
        0 ... x => {} //~ ERROR non-constant path
    };
}

The //~ is how we check the correct error is emitted - you wouldn't like random changes to rustc to cause the test to break on an unrelated error (this was more important before 1.0, but still occurs).

Anyway, your change causes 3 errors to be emitted in that case, and that is just ugly.

[alexispurslane]

Ok whoops! Thanks for explaining.

[alexispurslane]

The second commit gives the proper behavior. As I said in #28178, there are two things wrong with the code, so two errors are necessary, but the third error (the ICE) is not.

[alexispurslane]

OK, @nikomatsakis I implemented that. It works just fine. BTW, is there any way I can familiarize myself more with the Rust codebase? This is one of my first pull requests to Rust, so I don't have much domain-specific knowledge.

[nikomatsakis]

@ChristopherDumas (sorry for the delay)

OK, @nikomatsakis I implemented that. It works just fine.

Looks great, thanks. Can you maybe squash the commits? Most of the history doesn't seem so relevant anymore.

BTW, is there any way I can familiarize myself more with the Rust codebase? This is one of my first pull requests to Rust, so I don't have much domain-specific knowledge.

Hmm, I'm not sure of any great resource. I guess it's our job to let you know about these things :)

[nikomatsakis]

(our job as reviewers, I mean)

[alexispurslane]

@nikomatsakis Sorry about my delay, I'll squash the commits.

[alexispurslane]

@nikomatsakis I've squashed all the commits, and the tests are passing.

[alexispurslane]

@nikomatsakis I added a test, but since there are two errors on the same line, it fails. Do you know how to fix this?

[nikomatsakis]

@ChristopherDumas yes, you can do this:

/* offending-line */
//~^ ERROR error1
//~| ERROR error2

The ^ means "on the line above", and the | means "same line as the previous ERROR.

Just for reference, you can put any number of ^, so //~^^ means "two lines above" and so forth.

[alexispurslane]

@nikomatsakis Awesome! Thanks!

[nikomatsakis]

@bors r+

[bors]

📌 Commit 175a642 has been approved by nikomatsakis

[alexispurslane]

@nikomatsakis Is there a manual for the testing stuff that you showed me?

[bors]

🔒 Merge conflict

[bors]

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

[alexispurslane]

Merge conflict!

[alexispurslane]

@nikomatsakis Can you make bors retry?

[alexcrichton]

@bors: r=nikomatsakis 4611308

[bors]

⌛ Testing commit 4611308 with merge a690a2d...

[bors]

💔 Test failed - auto-win-msvc-64-opt

[alexispurslane]

@alexcrichton Why is bors' homu test failing?

[jonas-schievink]

failures:
    net::udp::tests::test_read_timeout
    net::udp::tests::test_read_with_timeout

Looks like a spurious failure, not caused by your PR

[arielb1]

@bors retry

@ChristopherDumas our buildbot is glitchy.
@alexcrichton what's the Geiger counter results on the buildbot?

[bors]

⌛ Testing commit 4611308 with merge 483600e...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt