Make resolve more perseverant with indeterminate imports. #26242

elinorbgr · 2015-06-12T09:56:16Z

This changes the behavior of the resolver from giving up at the first indeterminate import to trying to resolve everything in the current module.

It should make the resolving independent of the order in which the use statements are written in the source file (which is not the case currently).

I am currently running make check-stage1 on my computer, takes quite a long time, but no error yet. I still tested manually the example from #18083.

Fixes #18083.

rust-highfive · 2015-06-12T09:56:29Z

r? @alexcrichton

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

alexcrichton · 2015-06-12T17:07:07Z

src/librustc_resolve/resolve_imports.rs

-                                          import_directive.subclass),
-                                      help);
-                    self.resolver.resolve_error(span, &msg[..]);
+            let mut indeterminate = false;


Instead of a boolean flag, could this just run the push in the Indeterminate arm followed by a continue?

At this point imports is borrowed by import_directive, thus I cannot call .swap_remove(..). This is why I had to add a scope around the whole match { }.

But maybe there is a more elegant way to reorder the whole thing, need to think about it.

alexcrichton · 2015-06-12T17:09:36Z

Nice! Resolve changes can often be quite subtle, so I'd like to get another set of eyes on this at least. Also, I think it'd be helpful to get a crater run just to be 100% certain this doesn't cause any regressions.

cc @nrc, @brson

elinorbgr · 2015-06-12T17:58:12Z

Updated the commit with a cleaner organization than with a boolean flag.

And @alexcrichton , I do hope it does not cause any breakage, as it's supposed to make the resolver accept strictly more configurations. 😅

elinorbgr · 2015-06-12T18:27:59Z

I've been pointed on IRC that this might fix #4865 as well, and indeed the last example of the issue compiles fine with my fix.

In this case, the issue was that rustc does not resolve imports from a module containing glob imports that have not be resolved (

rust/src/librustc_resolve/resolve_imports.rs

Lines 457 to 459 in d7f5fa4

    
           // If there is an unresolved glob at this point in the 
        
           // containing module, bail out. We don't know enough to be 
        
           // able to resolve this import.

) and thus each module was waiting for the other to resolve c::*;. With my patch they don't wait any more.

brson · 2015-06-12T18:54:50Z

Nice! It will be great to get rid of some of these long-standing problems, but we have to be really careful since we're defining new semantics for a notoriously difficult pass.

I would feel more confident with more test cases. #4865 has quite a few, and many of them at least look subtly different. Can we add all of them?

elinorbgr · 2015-06-12T19:13:33Z

@brson You were right to be cautious, my patch only fix the last example of #4865. The other ones are still blocked, I think it's an other bug, specifically linked to glob imports. I guess I'll probably look at it as well, now that I'm here.

elinorbgr · 2015-06-12T19:27:01Z

Okay, I've spotted the blocking behavior for the rest of #4865 :

rustc will not resolve a glob import if target module still have unresolved imports
rustc will not resolve an import if the owning module of the target still have unresolved glob imports, unless it already found a result in both namespaces (not sure about why there is this exception, to be honest)

Starting from this, it is obvious how to create a loop and block the algorithm.

But I'm afraid the fix for this issue will be far less simple than my current fix (but it is still a different bug, independent of #18083 ).

brson · 2015-06-12T19:44:57Z

@vberger Thanks for the investigation. Can you add the 1 working case still?

We should probably have @pcwalton review this since he wrote much of resolve. Maybe @pnkfelix as well. Since this is an important change it should probably be escalated so @rust-lang/lang is aware of it. cc @nrc @nikomatsakis @aturon

pcwalton · 2015-06-12T21:34:21Z

Yeah, this looks OK to me.

nrc · 2015-06-23T13:33:52Z

ping @alexcrichton

(I think it looks fine to me, btw)

brson · 2015-06-23T20:04:09Z

I'll run this through crater.

brson · 2015-06-24T19:23:03Z

Toolchains are built. Building crates now.

brson · 2015-06-25T19:23:52Z

Crater is not thrilled!

nrc · 2015-06-25T19:28:19Z

@vberger do you think this can be fixed, or do the crater fails kill this PR?

elinorbgr · 2015-06-25T19:38:33Z

@nrc I'm looking into this, I need to understand why this fails like this before concluding anything.

elinorbgr · 2015-06-26T09:13:42Z

Okay, I think I found a minimal example of the breakage:

pub use foo::Foo;

use Foo as FooBar;

mod foo {
    pub use self::bar::*;
    mod bar {
        pub struct Foo;
    }
}

With my PR? this breaks on use Foo as FooBar; which is qualified as unresolved.

Apparently, the algorithm considers that, as Foo is a direct import and there is no glob import in the module, the it must be already resolved. It is the case with the current algorithm (provided use foo::Foo; is made before use Foo as FooBar;), but is no longer true with my change.

To make all tihs work, we'll need to make rustc declare imports as "unresolved" less quickly. I think I have an idea about that.

elinorbgr · 2015-06-26T13:55:08Z

Okay, this is very troubling. I made this example, which has the same structure as the failing import of Hyper:

mod bar {
    pub use self::middle::*;

    mod middle {
        pub use self::baz::Baz;

        mod baz {
            pub enum Baz {
                Baz1,
                Baz2
            }
        }
    }
}

mod foo {
    use bar::Baz::{Baz1, Baz2};
}

But it fails on current nightly, while Hyper compiles...

Anyway, overall the resolve algorithm seems to depend on the linear order of resolve (at several points, it decides that if the resolution is indeterminate, then the import must be failed. My above example is an example of one of these cases.

If we want to change this, I'll certainly need more thought. Still, I don't give up yet.

When encountering an indeterminate import, still try to resolve the following imports of the module. This avoids dead-lock-like situations where 2 modules are blocked, each waiting for the resolution of the other. Fixes rust-lang#18083.

elinorbgr · 2015-06-28T16:50:52Z

The problem was that some other parts of the algorithm were doing some kind of a "early return", by marking a import as unresolved when it is in fact undeterminate. With the previous ordered approach it was completely valid, as these imports would never have been resolved anyway. It is no longer the case with my change, so I updated them to return Undeterminate instead, and rely on the main loop of resolve_imports() to stop us (which it does pretty well).

This patch successfully passes make check-stage2-rpass, and compiles successfully rand and hyper, which where the two root breakages crater found previously.

elinorbgr · 2015-06-28T16:52:00Z

ping @alexcrichton @brson : r? and I guess it'll need an other run of crater ?

alexcrichton · 2015-06-29T17:24:14Z

Yeah let's run this through crater again. I believe @brson is on vacation right now, but he can schedule a new run once he's back.

brson · 2015-07-06T19:21:58Z

Started new crater run.

brson · 2015-07-06T23:35:41Z

Crater says all good

brson · 2015-07-06T23:35:51Z

@bors r+

bors · 2015-07-06T23:35:52Z

📌 Commit 255ce47 has been approved by brson

brson · 2015-07-06T23:37:08Z

Awesome work @vberger. Thanks for sticking it out.

bors · 2015-07-07T06:06:27Z

⌛ Testing commit 255ce47 with merge 82865ff...

bors · 2015-07-07T07:34:55Z

💔 Test failed - auto-linux-64-nopt-t

elinorbgr · 2015-07-08T17:17:32Z

Hmm, okay. Due to moving the handling of unresolved imports in the main loop, the error messages generated changed (an broke some cfail test).
It overall makes the error messages less helpful than before, so it'll need ore work on this side.

I don't have a lot of time right now, but I'll try to improve during the next days.

elinorbgr · 2015-07-14T08:30:51Z

Okay, most of the breakage in the tests is due to several error message changing slightly, for example, both Unresolved import .... There is no ..in... and Unresolved import .... Could not find ..in... are merged into Unresolved import ...., which I don't think is a big problem.

I'm a little more perplex about the Maybe you meant foo::* that appears regularly. Because if use foo; failed, there is no reason use foo::*; will not. So I'm slightly tempted to simply merge it into Unresolved import.

On the other hand, other error messages got merged into it, and we probably don't want to lose them: Maybe a missing extern crate and Did you mean ... ?. They can provide useful indications.

And, last but not least, it appears my assumption about the invariant of the refcell module.imports was wrong : the test use self::*; causes an ICE because of a double borrow of a refcell. I'm not sure it's exactly this, but I'm looking into it.

Now, I definitely see what you meant about resolve changes being subtle @alexcrichton 😲

bors · 2015-07-17T08:07:10Z

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

Gankra · 2015-07-27T02:21:59Z

@vberger what's your status?

elinorbgr · 2015-07-27T06:57:51Z

@gankro I must admit this is getting much more troublesome than I first expected, and I currently don't have a huge lot of time to spend on this.

I'll close it for now. I'll create a new PR if I manage to put something up.

@alexcrichton

(This is a second try at #26242. This time I think things should be ok.) The current algorithm handling import resolutions works sequentially, handling imports in the order they appear in the source file, and blocking/bailing on the first one generating an error/being unresolved. This can lead to situations where the order of the `use` statements can make the difference between "this code compiles" and "this code fails on an unresolved import" (see #18083 for example). This is especially true when considering glob imports. This PR changes the behaviour of the algorithm to instead try to resolve all imports in a module. If one fails, it is recorded and the next one is tried (instead of directly giving up). Also, all errors generated are stored (and not reported directly). The main loop of the algorithms guaranties that the algorithm will always finish: if a round of resolution does not resolve anything new, we are stuck and give up. At this point, the new version of the algorithm will display all errors generated by the last round of resolve. This way we are sure to not silence relevant errors or help messages, but also to not give up too early. **As a consequence, the import resolution becomes independent of the order in which the `use` statements are written in the source files.** I personally don't see any situations where this could be a problem, but this might need some thought. I passed `rpass` and `cfail` tests on my computer, and now am compiling a full stage2 compiler to ensure the crates reporting errors in my previous attempts still build correctly. I guess once I have checked it, this will need a crater run? Fixes #18083. r? @alexcrichton , cc @nrc @brson

rust-highfive assigned alexcrichton Jun 12, 2015

elinorbgr changed the title ~~Make revolve more perseverant with indeterminate imports.~~ Make resolve more perseverant with indeterminate imports. Jun 12, 2015

elinorbgr force-pushed the more_perseverant_resolve branch from acb101b to cec1132 Compare June 12, 2015 14:39

alexcrichton reviewed Jun 12, 2015
View reviewed changes

elinorbgr force-pushed the more_perseverant_resolve branch from cec1132 to 90ee272 Compare June 12, 2015 17:53

brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 12, 2015

brson mentioned this pull request Jun 12, 2015

Re-exporting items using a glob breaks if the module imports another module #4865

Closed

elinorbgr force-pushed the more_perseverant_resolve branch from 66517a6 to 255ce47 Compare June 28, 2015 16:45

alexcrichton mentioned this pull request Jul 8, 2015

Target Version rust-lang/rfcs#1147

Closed

elinorbgr closed this Jul 27, 2015

elinorbgr mentioned this pull request Jul 31, 2015

More perseverant resolve (second try) #27439

Merged

Make resolve more perseverant with indeterminate imports. #26242

Make resolve more perseverant with indeterminate imports. #26242

Conversation

elinorbgr commented Jun 12, 2015

rust-highfive commented Jun 12, 2015

alexcrichton Jun 12, 2015

Choose a reason for hiding this comment

elinorbgr Jun 12, 2015

Choose a reason for hiding this comment

elinorbgr Jun 12, 2015

Choose a reason for hiding this comment

alexcrichton Jun 12, 2015

Choose a reason for hiding this comment

alexcrichton commented Jun 12, 2015

elinorbgr commented Jun 12, 2015

elinorbgr commented Jun 12, 2015

brson commented Jun 12, 2015

elinorbgr commented Jun 12, 2015

elinorbgr commented Jun 12, 2015

brson commented Jun 12, 2015

pcwalton commented Jun 12, 2015

nrc commented Jun 23, 2015

brson commented Jun 23, 2015

brson commented Jun 24, 2015

brson commented Jun 25, 2015

nrc commented Jun 25, 2015

elinorbgr commented Jun 25, 2015

elinorbgr commented Jun 26, 2015

elinorbgr commented Jun 26, 2015

elinorbgr commented Jun 28, 2015

elinorbgr commented Jun 28, 2015

alexcrichton commented Jun 29, 2015

brson commented Jul 6, 2015

brson commented Jul 6, 2015

brson commented Jul 6, 2015

bors commented Jul 6, 2015

brson commented Jul 6, 2015

bors commented Jul 7, 2015

bors commented Jul 7, 2015

elinorbgr commented Jul 8, 2015

elinorbgr commented Jul 14, 2015

bors commented Jul 17, 2015

Gankra commented Jul 27, 2015

elinorbgr commented Jul 27, 2015