resolve: Explicit Set for detecting resolution cycles

[LorrensP-2158466]

View all comments

Instead of using the borrow_mut counter of a RefCell for a NameResolution for detecting cyclic imports during import resolution, we use an explicit recursion stack that keeps track of the current used NameResolutions.

Because of the upcoming parallelisation of the import resolution algorithm, the current way cannot used in a parallel context.

r? @petrochenkov

[petrochenkov]

It's clear that using a thread-local is more compact, but is it technically possible to pass the "active resolution" set explicitly as a parameter through all the relevant functions? We are already doing that for ignore_decl and ignore_import.

[LorrensP-2158466]

but is it technically possible to pass the "active resolution" set explicitly as a parameter through all the relevant functions?

I will try it and see how it looks.

[LorrensP-2158466]

To be honest, this gets quite cumbersome. There are a lot of functions that use the maybe_resolve_ident_... function. Currently i am just inserting that extra argument into everything where its needed, but its difficult for me to differentiate when it is needed and when not. Even then, like I said, a lot of places need this change then (not that we already did this with CmResolver).

So now i have:

pub(crate) fn maybe_resolve_ident_in_module<'r>(
    // ...
    cycle_detector: &mut ImportCycleDetector<'ra>,
){ ... }

(&mut ...) because of loops, otherwise we need cloning.

But this detecting is not needed everywhere, so we could do this:

pub(crate) fn maybe_resolve_ident_in_module<'r>(
    // ...
    cycle_detector: Option<&mut ImportCycleDetector<'ra>>,
){ ... }

But then we require reborrowing in closures and loops, which is the same story as with CmResolver, regardless of the &mut ... or a &mut BTreeSet inside of the Detector (which requires and extra lifetime parameter.

So before I complete this big refactor i have 2 questions:

• do you think this should be done? Because its technically possible as i see it now.
• if so, should i track everything that uses it, thus using a bare &mut ImportCycleDetector<'ra>. Or letting the caller decide when to track cycles in name resolutions, thus using Option<&mut ...>.

[petrochenkov]

There are a lot of functions that use the maybe_resolve_ident_... function.

I think we need the cycle detector in exactly the same cases as ignore_import: Option<Import<'ra>>, (14 functions have this argument).
Actually, both should be combinable into the same (optional) argument, unless I'm missing something.
If you merge the cycle detector into ignore_import, then you'll only need to change ignore_import's type in signatures and 2-3 places where it's actually constructed or inspected, and not just passed through, I think it's worth trying.

[LorrensP-2158466]

@rustbot ready

The cycle detector was also needed in macro resolution.

[LorrensP-2158466]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[LorrensP-2158466]

So i reproduced that failing CI job locally to try and find the stack trace, here is the minimal version:

// ... recursive behaviour of stack
resolve_ident_in_scope_set_inner at ident.rs:450:33 [opt]
resolve_ident_in_scope_set at ident.rs:397:14 [opt] [inlined]
resolve_path_with_ribs at ident.rs:2018:33 [opt]
maybe_resolve_path at ident.rs:1797:14 [opt]
path_accessible at macros.rs:1284:29 [opt]
expr_to_spanned_string at util.rs:87:24 [opt]
make_format_args at format.rs:188:40 [opt]
expand_format_args_impl at format.rs:1137:44 [opt]
expand_invoc at expand.rs:748:53 [opt] [inlined]
fully_expand_fragment at expand.rs:541:24 [opt]
// ... to start of stack

So it does seem that infinite recursion can happen anywhere, anytime. If you think that the cycle_detector should still be an argument to these resolve calls, then I think its best to just have a separate argument, instead of combining it with ignore_import. Otherwise we could just go with the TLS approach again.

[petrochenkov]

Let's return to the (scoped) TLS approach and just keep doing what we do on the main branch.

[LorrensP-2158466]

The more correct "is import" check would probably look like "resolution.non_glob_decl is some non-import, and resolution.glob_decl is none", but I'm still not sure if it would be fully correct.

yes, that is not entirely correct, I think.

this simple example:

mod a {
    pub use crate::b::X;
}

mod b {
    pub use crate::a::X;
}

Would have non_glob_decl and glob_decl as None when we receive the resolution of either import. But the single_imports are non-empty. As such, we keep going in circles.

I am inclined to think that if both non_glob_decl and glob_decl are None, we still do cycle_checking.

Edit: from debug:

 NameResolution { single_imports: {ImportData { kind: Single { source: X#0, target: X#0, decls: PerNS { value_ns: None, type_ns: None, macro_ns: None }, nested: false, id: NodeId(4), def_id: DefId(0:2 ~ single_cycle[5203]::a::{use#0}) }, root_id: NodeId(4), use_span: single_cycle.rs:5:5: 5:25 (#0), use_span_with_attributes: single_cycle.rs:5:5: 5:25 (#0), has_attributes: false, span: single_cycle.rs:5:13: 5:24 (#0), root_span: single_cycle.rs:5:13: 5:24 (#0), parent_scope: ParentScope { module: Def(Mod, DefId(0:1 ~ single_cycle[5203]::a)), expansion: expn0, macro_rules: Cell { value: Empty }, derives: [] }, module_path: [Segment { ident: crate#0, id: Some(NodeId(5)), has_generic_args: false, has_lifetime_args: false, args_span: no-location (#0) }, Segment { ident: b#0, id: Some(NodeId(6)), has_generic_args: false, has_lifetime_args: false, args_span: no-location (#0) }], imported_module: CmCell(Some(Module(Def(Mod, DefId(0:3 ~ single_cycle[5203]::b))))), vis: Public, vis_span: single_cycle.rs:5:5: 5:8 (#0), on_unknown_attr: None }}, non_glob_decl: None, glob_decl: None, orig_ident_span: single_cycle.rs:5:23: 5:24 (#0) }

[petrochenkov]

To clarify, by delaying the cycle check I also meant removing all the "is import" conditions.

[LorrensP-2158466]

@rustbot ready

[petrochenkov]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: b4f9537 (b4f9537fb47874f97164f0e216d412ec42846adf)
Base parent: 63f05e3 (63f05e3635171e7ac3f9ca78bad6c71052cda5a3)

[rust-timer]

Finished benchmarking commit (b4f9537): comparison URL.

Overall result: ❌ regressions - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.3%]	8
Regressions ❌ (secondary)	0.4%	[0.3%, 0.6%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[0.2%, 0.3%]	8

Max RSS (memory usage)

Results (primary 2.2%, secondary -3.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.2%	[2.2%, 2.2%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.8%	[-3.8%, -3.8%]	1
All ❌✅ (primary)	2.2%	[2.2%, 2.2%]	1

Cycles

Results (secondary 3.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.9%	[3.9%, 3.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

Results (primary -0.1%, secondary -0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.0%]	47
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.0%]	22
All ❌✅ (primary)	-0.1%	[-0.1%, -0.0%]	47

Bootstrap: 486.982s -> 487.285s (0.06%)
Artifact size: 393.22 MiB -> 393.55 MiB (0.08%)

[petrochenkov]

It looks acceptable now, unused-warnings is a stress benchmark with thousands of imports.
r=me after squashing commits.
@rustbot author

[LorrensP-2158466]

Much better, indeed. squashed to 1 commit, @rustbot ready.

r=me after squashing commits.

Can't do that :)

[petrochenkov]

@bors r+

[rust-bors]

📌 Commit 3056b8e has been approved by petrochenkov

It is now in the queue for this repository.