Skip to content

Conversation

Kobzol
Copy link
Member

@Kobzol Kobzol commented Sep 1, 2025

For #145354, we need PartialEq for TokenStream to "just work". However, due to the special comparison implementation that was used for InvisibleOrigin, this wasn't the case.

So I derived PartialEq for InvisibleOrigin, and used the previous special comparison logic only on the single place where it was actually required.

r? @petrochenkov

@rustbot rustbot added 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. labels Sep 1, 2025
@rust-log-analyzer

This comment has been minimized.

@Kobzol Kobzol force-pushed the invisible-origin-eq branch from 4ee338b to aed6884 Compare September 1, 2025 14:25
@petrochenkov petrochenkov 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 Sep 1, 2025
@Kobzol Kobzol marked this pull request as ready for review September 2, 2025 13:18
@petrochenkov
Copy link
Contributor

CI is green, I think the panicking PartialEq for Delimiter can be removed now.
@rustbot author

@rustbot rustbot 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 Sep 2, 2025
@Kobzol
Copy link
Member Author

Kobzol commented Sep 2, 2025

Thanks for taking a look! Based on what you commented, it looks like you don't think that eq_special_invisible_origin is needed at all. However, if I remove all usages of this function, then the panicking PartialEq will start kicking in (which is why I introduced this function in the first place). So there would be different behavior after my PR, although it might not be important, e.g.:

   2: <rustc_ast::token::Delimiter as core::cmp::PartialEq>::eq
   3: <rustc_ast_pretty::pprust::state::State as rustc_ast_pretty::pprust::state::PrintState>::print_mac_common
   4: <rustc_ast_pretty::pprust::state::State as rustc_ast_pretty::pprust::state::PrintState>::print_tt
   5: <rustc_ast_pretty::pprust::state::State as rustc_ast_pretty::pprust::state::PrintState>::print_tts
   6: <rustc_ast_pretty::pprust::state::State as rustc_ast_pretty::pprust::state::PrintState>::tts_to_string
   7: rustc_ast_pretty::pprust::tts_to_string
   8: rustc_builtin_macros::source_util::expand_stringify

In other words, if I remove all the special comparisons on which you commented, the PartialEq would panic. Is that fine, and should I remove the panic and thus change the comparison behavior?

@petrochenkov
Copy link
Contributor

petrochenkov commented Sep 2, 2025

if I remove all usages of this function, then the panicking PartialEq will start kicking in

The panic in PartialEq for Delimiter is too eager, it should only panic if both sides are Delimiter::Invisible.
For e.g. Delimiter::Parentheses == Delimiter::Invisible(anything) it's quite clear that the result is false and there's no change in behavior.

Based on what you commented, it looks like you don't think that eq_special_invisible_origin is needed at all.

Right.

@Kobzol Kobzol force-pushed the invisible-origin-eq branch from aed6884 to 51566a2 Compare September 2, 2025 14:39
@Kobzol
Copy link
Member Author

Kobzol commented Sep 2, 2025

Aah, of course, you're right, sorry. Ok, I cleaned up the first commit, and added the panicking PartialEq in a second WIP commit, just to make sure that CI will still be green. If CI is green, I'll remove the second commit.

@Kobzol Kobzol force-pushed the invisible-origin-eq branch from 51566a2 to a8537ab Compare September 2, 2025 16:08
@Kobzol
Copy link
Member Author

Kobzol commented Sep 2, 2025

Ok, CI is green, so hopefully no panics should be triggered, at least in the default test suite. Removing the second WIP commit.

@rustbot ready

@rustbot 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 Sep 2, 2025
@Kobzol Kobzol changed the title Implement comparison of InvisibleOrigin without PartialEq Derive PartialEq for InvisibleOrigin Sep 2, 2025
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 2, 2025

📌 Commit a8537ab has been approved by petrochenkov

It is now in the queue for this repository.

@bors 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 Sep 2, 2025
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 2, 2025
…ochenkov

Derive `PartialEq` for `InvisibleOrigin`

For rust-lang#145354, we need `PartialEq` for `TokenStream` to "just work". However, due to the special comparison implementation that was used for `InvisibleOrigin`, this wasn't the case.

So I derived `PartialEq` for `InvisibleOrigin`, and used the previous special comparison logic only on the single place where it was actually required.

r? `@petrochenkov`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 2, 2025
…ochenkov

Derive `PartialEq` for `InvisibleOrigin`

For rust-lang#145354, we need `PartialEq` for `TokenStream` to "just work". However, due to the special comparison implementation that was used for `InvisibleOrigin`, this wasn't the case.

So I derived `PartialEq` for `InvisibleOrigin`, and used the previous special comparison logic only on the single place where it was actually required.

r? ``@petrochenkov``
bors added a commit that referenced this pull request Sep 2, 2025
Rollup of 8 pull requests

Successful merges:

 - #139113 (unstable book: in a sanitizer example, check the code)
 - #145823 (editorconfig: don't use nonexistent syntax)
 - #145962 (Ensure we emit an allocator shim when only some crate types need one)
 - #146032 (Explicity disable LSX feature for `loongarch64-unknown-none` target)
 - #146090 (Derive `PartialEq` for `InvisibleOrigin`)
 - #146120 (Correct typo in `rustc_errors` comment)
 - #146121 (fix: Filter suggestion parts that match existing code)
 - #146134 (llvm: nvptx: Layout update to match LLVM)

r? `@ghost`
`@rustbot` modify labels: rollup
tgross35 added a commit to tgross35/rust that referenced this pull request Sep 3, 2025
…ochenkov

Derive `PartialEq` for `InvisibleOrigin`

For rust-lang#145354, we need `PartialEq` for `TokenStream` to "just work". However, due to the special comparison implementation that was used for `InvisibleOrigin`, this wasn't the case.

So I derived `PartialEq` for `InvisibleOrigin`, and used the previous special comparison logic only on the single place where it was actually required.

r? ```@petrochenkov```
tgross35 added a commit to tgross35/rust that referenced this pull request Sep 3, 2025
…ochenkov

Derive `PartialEq` for `InvisibleOrigin`

For rust-lang#145354, we need `PartialEq` for `TokenStream` to "just work". However, due to the special comparison implementation that was used for `InvisibleOrigin`, this wasn't the case.

So I derived `PartialEq` for `InvisibleOrigin`, and used the previous special comparison logic only on the single place where it was actually required.

r? ````@petrochenkov````
tgross35 added a commit to tgross35/rust that referenced this pull request Sep 3, 2025
…ochenkov

Derive `PartialEq` for `InvisibleOrigin`

For rust-lang#145354, we need `PartialEq` for `TokenStream` to "just work". However, due to the special comparison implementation that was used for `InvisibleOrigin`, this wasn't the case.

So I derived `PartialEq` for `InvisibleOrigin`, and used the previous special comparison logic only on the single place where it was actually required.

r? `````@petrochenkov`````
bors added a commit that referenced this pull request Sep 3, 2025
Rollup of 9 pull requests

Successful merges:

 - #143725 (core: add Peekable::next_if_map)
 - #145209 (Stabilize `path_add_extension`)
 - #145750 (raw_vec.rs: Remove superfluous fn alloc_guard)
 - #145962 (Ensure we emit an allocator shim when only some crate types need one)
 - #145963 (Add LSX accelerated implementation for source file analysis)
 - #146054 (add `#[must_use]` to `array::repeat`)
 - #146090 (Derive `PartialEq` for `InvisibleOrigin`)
 - #146120 (Correct typo in `rustc_errors` comment)
 - #146131 (rustdoc-search: add test case for indexing every item type)

r? `@ghost`
`@rustbot` modify labels: rollup
jhpratt added a commit to jhpratt/rust that referenced this pull request Sep 3, 2025
…ochenkov

Derive `PartialEq` for `InvisibleOrigin`

For rust-lang#145354, we need `PartialEq` for `TokenStream` to "just work". However, due to the special comparison implementation that was used for `InvisibleOrigin`, this wasn't the case.

So I derived `PartialEq` for `InvisibleOrigin`, and used the previous special comparison logic only on the single place where it was actually required.

r? ``````@petrochenkov``````
bors added a commit that referenced this pull request Sep 3, 2025
Rollup of 15 pull requests

Successful merges:

 - #143725 (core: add Peekable::next_if_map)
 - #145209 (Stabilize `path_add_extension`)
 - #145750 (raw_vec.rs: Remove superfluous fn alloc_guard)
 - #145962 (Ensure we emit an allocator shim when only some crate types need one)
 - #145963 (Add LSX accelerated implementation for source file analysis)
 - #146054 (add `#[must_use]` to `array::repeat`)
 - #146090 (Derive `PartialEq` for `InvisibleOrigin`)
 - #146120 (Correct typo in `rustc_errors` comment)
 - #146127 (Rename `ToolRustc` to `ToolRustcPrivate`)
 - #146131 (rustdoc-search: add test case for indexing every item type)
 - #146133 (Revert "Make `lto` and `linker-plugin-lto` work the same for `compiler_builtins`)
 - #146134 (llvm: nvptx: Layout update to match LLVM)
 - #146136 (docs(std): add missing closing code block fences in doc comments)
 - #146137 (Disallow frontmatter in `--cfg` and `--check-cfg` arguments)
 - #146140 (compiletest: cygwin follows windows in using PATH for dynamic libraries)

r? `@ghost`
`@rustbot` modify labels: rollup
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 3, 2025
…ochenkov

Derive `PartialEq` for `InvisibleOrigin`

For rust-lang#145354, we need `PartialEq` for `TokenStream` to "just work". However, due to the special comparison implementation that was used for `InvisibleOrigin`, this wasn't the case.

So I derived `PartialEq` for `InvisibleOrigin`, and used the previous special comparison logic only on the single place where it was actually required.

r? ```````@petrochenkov```````
bors added a commit that referenced this pull request Sep 3, 2025
Rollup of 16 pull requests

Successful merges:

 - #143725 (core: add Peekable::next_if_map)
 - #145209 (Stabilize `path_add_extension`)
 - #145342 (fix drop scope for `super let` bindings within `if let`)
 - #145750 (raw_vec.rs: Remove superfluous fn alloc_guard)
 - #145962 (Ensure we emit an allocator shim when only some crate types need one)
 - #145963 (Add LSX accelerated implementation for source file analysis)
 - #146054 (add `#[must_use]` to `array::repeat`)
 - #146090 (Derive `PartialEq` for `InvisibleOrigin`)
 - #146120 (Correct typo in `rustc_errors` comment)
 - #146127 (Rename `ToolRustc` to `ToolRustcPrivate`)
 - #146133 (Revert "Make `lto` and `linker-plugin-lto` work the same for `compiler_builtins`)
 - #146134 (llvm: nvptx: Layout update to match LLVM)
 - #146136 (docs(std): add missing closing code block fences in doc comments)
 - #146137 (Disallow frontmatter in `--cfg` and `--check-cfg` arguments)
 - #146140 (compiletest: cygwin follows windows in using PATH for dynamic libraries)
 - #146156 (miri subtree update)

r? `@ghost`
`@rustbot` modify labels: rollup
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 4, 2025
…ochenkov

Derive `PartialEq` for `InvisibleOrigin`

For rust-lang#145354, we need `PartialEq` for `TokenStream` to "just work". However, due to the special comparison implementation that was used for `InvisibleOrigin`, this wasn't the case.

So I derived `PartialEq` for `InvisibleOrigin`, and used the previous special comparison logic only on the single place where it was actually required.

r? ````````@petrochenkov````````
bors added a commit that referenced this pull request Sep 4, 2025
Rollup of 24 pull requests

Successful merges:

 - #140459 (Add `read_buf` equivalents for positioned reads)
 - #143725 (core: add Peekable::next_if_map)
 - #145209 (Stabilize `path_add_extension`)
 - #145342 (fix drop scope for `super let` bindings within `if let`)
 - #145750 (raw_vec.rs: Remove superfluous fn alloc_guard)
 - #145827 (On unused binding or binding not present in all patterns, suggest potential typo of unit struct/variant or const)
 - #145932 (Allow `inline(always)` with a target feature behind a unstable feature `target_feature_inline_always`.)
 - #145962 (Ensure we emit an allocator shim when only some crate types need one)
 - #145963 (Add LSX accelerated implementation for source file analysis)
 - #146054 (add `#[must_use]` to `array::repeat`)
 - #146090 (Derive `PartialEq` for `InvisibleOrigin`)
 - #146112 (don't uppercase error messages)
 - #146120 (Correct typo in `rustc_errors` comment)
 - #146124 (Test `rustc-dev` in `distcheck`)
 - #146127 (Rename `ToolRustc` to `ToolRustcPrivate`)
 - #146131 (rustdoc-search: add test case for indexing every item type)
 - #146134 (llvm: nvptx: Layout update to match LLVM)
 - #146136 (docs(std): add missing closing code block fences in doc comments)
 - #146137 (Disallow frontmatter in `--cfg` and `--check-cfg` arguments)
 - #146140 (compiletest: cygwin follows windows in using PATH for dynamic libraries)
 - #146150 (fix(rustdoc): match rustc `--emit` precedence )
 - #146155 (Make bootstrap self test parallel)
 - #146161 ([rustdoc] Uncomment code to add scraped rustdoc examples in loaded paths)
 - #146172 (triagebot: configure some pings when certain attributes are used)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 57e8a53 into rust-lang:master Sep 4, 2025
10 checks passed
@rustbot rustbot added this to the 1.91.0 milestone Sep 4, 2025
rust-timer added a commit that referenced this pull request Sep 4, 2025
Rollup merge of #146090 - Kobzol:invisible-origin-eq, r=petrochenkov

Derive `PartialEq` for `InvisibleOrigin`

For #145354, we need `PartialEq` for `TokenStream` to "just work". However, due to the special comparison implementation that was used for `InvisibleOrigin`, this wasn't the case.

So I derived `PartialEq` for `InvisibleOrigin`, and used the previous special comparison logic only on the single place where it was actually required.

r? `````````@petrochenkov`````````
@Kobzol Kobzol deleted the invisible-origin-eq branch September 4, 2025 06:11
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Sep 5, 2025
Rollup of 24 pull requests

Successful merges:

 - rust-lang/rust#140459 (Add `read_buf` equivalents for positioned reads)
 - rust-lang/rust#143725 (core: add Peekable::next_if_map)
 - rust-lang/rust#145209 (Stabilize `path_add_extension`)
 - rust-lang/rust#145342 (fix drop scope for `super let` bindings within `if let`)
 - rust-lang/rust#145750 (raw_vec.rs: Remove superfluous fn alloc_guard)
 - rust-lang/rust#145827 (On unused binding or binding not present in all patterns, suggest potential typo of unit struct/variant or const)
 - rust-lang/rust#145932 (Allow `inline(always)` with a target feature behind a unstable feature `target_feature_inline_always`.)
 - rust-lang/rust#145962 (Ensure we emit an allocator shim when only some crate types need one)
 - rust-lang/rust#145963 (Add LSX accelerated implementation for source file analysis)
 - rust-lang/rust#146054 (add `#[must_use]` to `array::repeat`)
 - rust-lang/rust#146090 (Derive `PartialEq` for `InvisibleOrigin`)
 - rust-lang/rust#146112 (don't uppercase error messages)
 - rust-lang/rust#146120 (Correct typo in `rustc_errors` comment)
 - rust-lang/rust#146124 (Test `rustc-dev` in `distcheck`)
 - rust-lang/rust#146127 (Rename `ToolRustc` to `ToolRustcPrivate`)
 - rust-lang/rust#146131 (rustdoc-search: add test case for indexing every item type)
 - rust-lang/rust#146134 (llvm: nvptx: Layout update to match LLVM)
 - rust-lang/rust#146136 (docs(std): add missing closing code block fences in doc comments)
 - rust-lang/rust#146137 (Disallow frontmatter in `--cfg` and `--check-cfg` arguments)
 - rust-lang/rust#146140 (compiletest: cygwin follows windows in using PATH for dynamic libraries)
 - rust-lang/rust#146150 (fix(rustdoc): match rustc `--emit` precedence )
 - rust-lang/rust#146155 (Make bootstrap self test parallel)
 - rust-lang/rust#146161 ([rustdoc] Uncomment code to add scraped rustdoc examples in loaded paths)
 - rust-lang/rust#146172 (triagebot: configure some pings when certain attributes are used)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

5 participants