Convert the rest of the visitors to use `VisitorResult` #121576

Jarcho · 2024-02-25T00:51:28Z

Continuing from #121256.

rustbot · 2024-02-25T00:51:36Z

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-02-25T00:51:38Z

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred in const_evaluatable.rs

cc @BoxyUwU

oli-obk · 2024-02-25T06:42:05Z

compiler/rustc_type_ir/src/visit.rs

@@ -47,6 +47,83 @@ use std::ops::ControlFlow;

 use crate::{self as ty, BoundVars, Interner, IntoKind, Lrc, TypeFlags};

+/// Similar to the `Try` trait, but also implemented for `()`.


Move this to rustc_data_structures, so you can avoid all the new crate dependencies (even if you have to add some, they'll already be there transitively)

rustc_type_ir is already a transitive dependency for all of them via rustc_ast (or rustc_hir, which depends on rustc_ast).

huh. Why does ast depend on type_ir

Ah

rust/compiler/rustc_ast/Cargo.toml

Line 17 in e9f9594

# For Mutability and Movability, which could be uplifted into a common crate.

Let's avoid adding more direct deps and instead move this new trait and macros to data structures. It's not type system specific after all

What's with the nightly feature on rustc_type_ir and all the optional dependencies? Should I just ignore those or make the whole visit module depend on a feature?

Hmm I see your point. That's annoying. I guess it's the same with the movability and the mutability enums. If you feel really motivated rn, you could split those out into a new crate and get rid of the ast->type_ir dep

Just pulling them into their own crate is easy. Doing it in a way that's actually nice is probably going to be a fair bit of work.

The split is done. Please move it to rustc_ast_ir and adjust the dependencies. Where useful, reexports could help reduce the amount of direct dependencies on rustc_ast_ir

oli-obk

All the impls lgtm

bors · 2024-02-26T15:28:57Z

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

…-errors Split rustc_type_ir to avoid rustc_ast from depending on it unblocks rust-lang#121576 and resolves a FIXME in `rustc_ast`'s `Cargo.toml` The new crate is tiny, but it will get bigger in rust-lang#121576

Rollup merge of rust-lang#121695 - oli-obk:split_ty_utils, r=compiler-errors Split rustc_type_ir to avoid rustc_ast from depending on it unblocks rust-lang#121576 and resolves a FIXME in `rustc_ast`'s `Cargo.toml` The new crate is tiny, but it will get bigger in rust-lang#121576

Jarcho · 2024-03-02T20:31:08Z

@rustbot ready

oli-obk · 2024-03-02T21:42:32Z

compiler/rustc_ast/src/visit.rs

@@ -15,8 +15,8 @@

 use crate::ast::*;

-use core::ops::ControlFlow;
-
+use rustc_ast_ir::visit::VisitorResult;


Suggested change

use rustc_ast_ir::visit::VisitorResult;

pub use rustc_ast_ir::visit::VisitorResult;

And then get rid of all the direct dependencies on rustc_ast_ir

oli-obk · 2024-03-04T11:53:31Z

@bors r+

bors · 2024-03-04T11:53:33Z

📌 Commit e1e02a2 has been approved by oli-obk

It is now in the queue for this repository.

Convert the rest of the visitors to use `VisitorResult` Continuing from rust-lang#121256.

…llaumeGomez Rollup of 9 pull requests Successful merges: - rust-lang#120976 (constify a couple thread_local statics) - rust-lang#121576 (Convert the rest of the visitors to use `VisitorResult`) - rust-lang#121826 (Use root obligation on E0277 for some cases) - rust-lang#121928 (Extract an arguments struct for `Builder::then_else_break`) - rust-lang#121958 (Fix redundant import errors for preload extern crate) - rust-lang#121959 (Removing absolute path in proc-macro) - rust-lang#121968 (Don't run test_get_os_named_thread on win7) - rust-lang#121977 (Doc: Fix incorrect reference to integer in Atomic{Ptr,Bool}::as_ptr.) - rust-lang#121978 (Fix duplicated path in the "not found dylib" error) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2024-03-05T10:39:14Z

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

oli-obk · 2024-03-05T10:41:15Z

@bors p=1 bitrotty

please rebase, we'll land with priority next time

oli-obk · 2024-03-05T20:49:32Z

@bors r+

bors · 2024-03-05T20:49:34Z

📌 Commit 228eb38 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2024-03-05T21:11:02Z

⌛ Testing commit 228eb38 with merge b6d2d84...

bors · 2024-03-05T23:31:15Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing b6d2d84 to master...

rust-timer · 2024-03-06T01:20:09Z

Finished benchmarking commit (b6d2d84): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 644.259s -> 646.18s (0.30%)
Artifact size: 175.02 MiB -> 175.03 MiB (0.00%)

rustbot assigned wesleywiser Feb 25, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Feb 25, 2024

This comment has been minimized.

Sign in to view

Jarcho force-pushed the visitor3 branch from 88515ce to f2476ce Compare February 25, 2024 03:00

This comment has been minimized.

Sign in to view

Jarcho force-pushed the visitor3 branch from f2476ce to 1a6dd68 Compare February 25, 2024 03:21

This comment has been minimized.

Sign in to view

Jarcho force-pushed the visitor3 branch from 1a6dd68 to f3ecbeb Compare February 25, 2024 03:34

oli-obk reviewed Feb 25, 2024

View reviewed changes

oli-obk requested changes Feb 25, 2024

View reviewed changes

oli-obk mentioned this pull request Feb 27, 2024

Split rustc_type_ir to avoid rustc_ast from depending on it #121695

Merged

wesleywiser added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 28, 2024

Jarcho force-pushed the visitor3 branch from f3ecbeb to d0ee1a6 Compare March 2, 2024 17:35

This comment has been minimized.

Sign in to view

Jarcho force-pushed the visitor3 branch from d0ee1a6 to 87d2b3e Compare March 2, 2024 18:28

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 2, 2024

Jarcho mentioned this pull request Mar 2, 2024

Use ControlFlow in visitors. #121563

Merged

oli-obk unassigned wesleywiser Mar 2, 2024

oli-obk reviewed Mar 2, 2024

View reviewed changes

Jarcho force-pushed the visitor3 branch from 87d2b3e to e1e02a2 Compare March 3, 2024 19:16

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 4, 2024

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Mar 4, 2024

Rollup merge of rust-lang#121576 - Jarcho:visitor3, r=oli-obk

3c16447

Convert the rest of the visitors to use `VisitorResult` Continuing from rust-lang#121256.

GuillaumeGomez mentioned this pull request Mar 4, 2024

Rollup of 9 pull requests #121982

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 5, 2024

Jarcho added 4 commits March 5, 2024 12:38

Move visitor utils to rustc_ast_ir

5abfb37

Convert TypeVisitor and DefIdVisitor to use VisitorResult

be9b125

Convert SpannedTypeVisitor to use VisitorResult

ea9ae30

Convert ProofTreeVisitor to use VisitorResult

228eb38

Jarcho force-pushed the visitor3 branch from e1e02a2 to 228eb38 Compare March 5, 2024 18:31

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 5, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 5, 2024

bors merged commit b6d2d84 into rust-lang:master Mar 5, 2024
12 checks passed

rustbot added this to the 1.78.0 milestone Mar 5, 2024

bors mentioned this pull request Mar 5, 2024

bump itertools to 0.12 #121584

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert the rest of the visitors to use `VisitorResult` #121576

Convert the rest of the visitors to use `VisitorResult` #121576

Jarcho commented Feb 25, 2024

rustbot commented Feb 25, 2024

rustbot commented Feb 25, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

oli-obk Feb 25, 2024

Jarcho Feb 25, 2024

oli-obk Feb 25, 2024

oli-obk Feb 25, 2024

oli-obk Feb 25, 2024

Jarcho Feb 25, 2024

oli-obk Feb 25, 2024

Jarcho Feb 25, 2024

oli-obk Feb 28, 2024

oli-obk left a comment

bors commented Feb 26, 2024

This comment has been minimized.

Jarcho commented Mar 2, 2024

oli-obk Mar 2, 2024

Jarcho Mar 3, 2024

oli-obk commented Mar 4, 2024

bors commented Mar 4, 2024

bors commented Mar 5, 2024

oli-obk commented Mar 5, 2024

oli-obk commented Mar 5, 2024

bors commented Mar 5, 2024

bors commented Mar 5, 2024

bors commented Mar 5, 2024

rust-timer commented Mar 6, 2024

		@@ -47,6 +47,83 @@ use std::ops::ControlFlow;

		use crate::{self as ty, BoundVars, Interner, IntoKind, Lrc, TypeFlags};

		/// Similar to the `Try` trait, but also implemented for `()`.

	use rustc_ast_ir::visit::VisitorResult;
	pub use rustc_ast_ir::visit::VisitorResult;

Convert the rest of the visitors to use VisitorResult #121576

Convert the rest of the visitors to use VisitorResult #121576

Conversation

Jarcho commented Feb 25, 2024

rustbot commented Feb 25, 2024

rustbot commented Feb 25, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk left a comment

Choose a reason for hiding this comment

bors commented Feb 26, 2024

This comment has been minimized.

Jarcho commented Mar 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Mar 4, 2024

bors commented Mar 4, 2024

bors commented Mar 5, 2024

oli-obk commented Mar 5, 2024

oli-obk commented Mar 5, 2024

bors commented Mar 5, 2024

bors commented Mar 5, 2024

bors commented Mar 5, 2024

rust-timer commented Mar 6, 2024

Overall result: ❌ regressions - no action needed

Convert the rest of the visitors to use `VisitorResult` #121576

Convert the rest of the visitors to use `VisitorResult` #121576