Handle partial resolve cases

[edwin0cheng]

Another try to fix #2443 :

We resolve all imports every time in DefCollector::collect loop even it is resolved previously.
This is because other unresolved imports and macros will bring in another PerNs, so we can only assume that it has been partially resolved.

[matklad]

I wonder how this looks performance-wise. My gut feeling is that we'll need something more involved here, like tracking unresovled types/values/macros separatelly, as well as remembering name resolution results for prefixes (instead of resolving each path from the start every time).

[matklad]

Hm, another easy optimization we can do is to mark paths which are rooted at external crates as ReachedFixedPoint::Yes -- crate graphs can't change in other crates.

[edwin0cheng]

Hm, another easy optimization we can do is to mark paths which are rooted at external crates as ReachedFixedPoint::Yes -- crate graphs can't change in other crates.

Good idea. I added some easy optimizations:

• Skip adding it to unresolved if it is with external crates or external crates root
• Skip record import if the PerNs did not changed

like tracking unresovled types/values/macros separatelly, as well as remembering name resolution results for prefixes (instead of resolving each path from the start every time).

I understand the name resolution prefixes one (I would recommend adding general path cache in DefCollector in another PR), but why should we tracks unresolved types/values/macros separately ?

[matklad]

Hm, I feel uneasy about this change, for two reasons:

• it adds even more elements to already large and unreadable tuples. This bit of code needs a redesign, it's already (before this PR) way to complex.
• with this change, the unified unresolved_imports list doesn't actually make sense, as everything happens on per-ns basis.

It might be a good idea to check, how this work list is setup in rustc. What exactly is the state during iteration?

[edwin0cheng]

Hm, I feel uneasy about this change, for two reasons:

Well, these changes can of course be rejected because I don't have a general idea how to solve this problem and trying out to see what code will work. :)

It might be a good idea to check, how this work list is setup in rustc. What exactly is the state during iteration?

Sure, and you are right. rustc solve this problem very cleanly.

TLDR:

here are an over simplifer version of how rustc do (pseudo code):

fn resolve_imports() {
    while old_unresolved_imports.len() != unresolved_imports.len() {
        for item in old_unresolved_imports {
            let res = resolve_item(item);
            if res == RESOLVED {
                unresolved_imports.push(item);
            } else {
                resolved_imports.push(item)
            }
        }
    };
}

fn passes() {
    // Expand macro pass
    loop {
        resolve_imports();
        
        if resolve_and_expand_macros() == REACH_FIX_POINT {
            break;
        }
    }
    
    unresolved_imports.extends(resolved_imports);
    resolve_imports();
}

Details

An import item is added in add_import_directive, the namespace is stored in ImportDirect::subclass, which is empty in the beginning.

While the expanding loop start at fully_expand_fragment, it will call resolve_imports and try to expand macro invocations. resolve_imports just forward to ImportResolver::resolver_imports, and you can see it is a fixed-point algorithm.

After all expansions loop ends, for each crate, resolver.resolve_crate(krate) will be called, and it will call finalize_imports, basically what it do is resolve all imports again and do some checking and error reporting.

[matklad]

Thanks for this overview @edwin0cheng , it's really helpful!

So I think this are the main diff with our current mode:

1. Imports track fixed-point state per-ns, indeterminate = true is executed if at least one ns could be further resolved
2. Imports are tree-shaped. ImportDirective, I think, refers to a single segment of an import. This allows one to avoid repeatedly resolve the same path (which is something we do a lot)
3. An import is marked as Err(Determined) if we know it can't resolve eo anything

Notably, it still maintains a single list of unresolved imports!

From the points above, 1 affects correctness, while 2 and 3 are strictly performance optimizations.

I think we probably should start here with a solid foundation for 1.

PartialResolvedImport is sort-of goes in that direction, but I think it complicates the existing code, without solving the core problem (the fact, that we need to track three fixed-points per import).

I am out of steam for today to be able to concretely describe the next steps here, but I feel a set of pre-requsites is:

• just general refactorings, to make sure that we don't work with four-tuples everywhere
• some plumbing to make sure that we track ReachedFixedPoint per namespace
• initially, I think we still want to consider an imported resolved if at least one namespace is resolved (to avoid regressing perf)
• make sure that, when we resolve import in external crate, we mark all three nses as reached fixedpoint
• flipping the "resolved" condition for import from "at least one ns" to "all three ns"

[edwin0cheng]

I rebased and made some changes:

1. Added ImportDirective for replace the four-tuples
2. Added PartialResolvedImport::Indeterminate indicate it is not fully resolved.
3. Added checking whether if all namespace all resolved, mark it as PartialResolvedImport::Resolved
4. We made the resolve_imports itself do the fixpoint algorithm.
5. Although we mark PartialResolvedImport::Indeterminate resolved, (By pushing it in resolved_imports) we add another resolve_imports call after the resolve loop to resolve all indeterminate resolves again to catch all missing cases. (That's why the new tests could pass)

[matklad]

This looks great now!

bors r+

[edwin0cheng]

bors retry

[bors]

Already running a review

[bors]

Build succeeded

• TypeScript
• Rust