Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use ControlFlow in HIR Visitor #108598

Closed
wants to merge 1 commit into from
Closed

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Mar 1, 2023

This ended up significantly larger than I originally though it would be. Most changes are trivial signature changes plus code changes to make the function actually return the correct type.

Some visitors were changed to return their result as BreakTy rather than through a field. Most of these are from clippy, which is the main motivation behind this change, but a few are from rustc as well.

There are a few behavior in some diagnostics, but only in the case of unusual spans (e.g. multiple nodes having the same span). This should only cause changes when the diagnostic spans were already questionable in the first place.

@rustbot
Copy link
Collaborator

rustbot commented Mar 1, 2023

r? @wesleywiser

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

@rustbot 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 1, 2023
@rustbot
Copy link
Collaborator

rustbot commented Mar 1, 2023

Some changes occurred in need_type_info.rs

cc @lcnr

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

These commits modify the Cargo.lock file. Random changes to Cargo.lock can be introduced when switching branches and rebasing PRs.
This was probably unintentional and should be reverted before this PR is merged.

If this was intentional then you can ignore this comment.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 1, 2023

Looks like the clippy test suite is having ICEs now

@Jarcho Jarcho force-pushed the break_hir_visitor branch 3 times, most recently from ef3b880 to 32ea017 Compare March 1, 2023 15:55
@oli-obk
Copy link
Contributor

oli-obk commented Mar 1, 2023

Do you have a link to clippy discussions that motivated this change? Not that it's not desirable on its own, but seems good to have some links. You should also open a compiler team MCP, as this is definitely a major change 😆

@Jarcho Jarcho force-pushed the break_hir_visitor branch 2 times, most recently from cef18e9 to 936cbbe Compare March 1, 2023 17:06
@Jarcho
Copy link
Contributor Author

Jarcho commented Mar 1, 2023

No clippy discussion for this. Just observing how clippy breaks early from most visitors.

@bors
Copy link
Contributor

bors commented Mar 1, 2023

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

@oli-obk
Copy link
Contributor

oli-obk commented Mar 8, 2023

r? @oli-obk

@rustbot rustbot assigned oli-obk and unassigned wesleywiser Mar 8, 2023
@pnkfelix
Copy link
Member

Discussed in today's T-compiler triage meeting.

Now this did have an MCP which was seconded, that's rust-lang/compiler-team#597

but the MCP second did note that there may be room for adjusting what the actual solution here looks like.

There may be good material to review in the Zulip stream associated with the MCP

@oli-obk oli-obk added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 11, 2023
@oli-obk
Copy link
Contributor

oli-obk commented May 11, 2023

Kind of blocked on rust-lang/libs-team#187

@Dylan-DPC Dylan-DPC added S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels May 20, 2023
@Dylan-DPC Dylan-DPC 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-ACP Status: PR has an ACP and is waiting for the ACP to complete. labels Feb 6, 2024
@Dylan-DPC
Copy link
Member

The ACP was closed

@Jarcho
Copy link
Contributor Author

Jarcho commented Feb 14, 2024

With the ACP closed that rules out using Try for this. We can still define our own trait e.g.

trait VisitorResult {
  type BreakTy;
  fn continue() -> Self;
  fn from_break_ty(_: Self::BreakTy) -> Self;
}
impl VisitorResult for () {
  type BreakTy = !;
  fn continue() -> Self {
    ()
  }
  fn from_break_ty(_: !) -> Self {
    ()
  }
}
impl<T: Try<Output = ()>> VisitorResult for T {
  type BreakTy = T::Residual;
  fn continue() -> Self {
    Self::from_output(())
  }
  fn from_break_ty(x: Self::BreakTy) -> Self {
    Self::from_residual(x)
  }
}

This does rule out using ? in functions generic over the result type, but those are really only part of the visitor impl and a try-like macro can be made for that.

saethlin added a commit to saethlin/rust that referenced this pull request Feb 20, 2024
Allow AST and HIR visitors to return `ControlFlow`

Alternative to rust-lang#108598.

Since rust-lang/libs-team#187 was rejected, this implements our own version of the `Try` trait (`VisitorResult`) and the `try` macro (`try_visit`). Since this change still allows visitors to return `()`, no changes have been made to the existing ones. They can be done in a separate PR.
Noratrieb added a commit to Noratrieb/rust that referenced this pull request Feb 20, 2024
Allow AST and HIR visitors to return `ControlFlow`

Alternative to rust-lang#108598.

Since rust-lang/libs-team#187 was rejected, this implements our own version of the `Try` trait (`VisitorResult`) and the `try` macro (`try_visit`). Since this change still allows visitors to return `()`, no changes have been made to the existing ones. They can be done in a separate PR.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 20, 2024
Rollup merge of rust-lang#121256 - Jarcho:visitor2, r=oli-obk

Allow AST and HIR visitors to return `ControlFlow`

Alternative to rust-lang#108598.

Since rust-lang/libs-team#187 was rejected, this implements our own version of the `Try` trait (`VisitorResult`) and the `try` macro (`try_visit`). Since this change still allows visitors to return `()`, no changes have been made to the existing ones. They can be done in a separate PR.
@Jarcho Jarcho closed this Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants