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

Always omit zero disambiguators on crate-roots #69

Conversation

michaelwoerister
Copy link
Member

This PR makes rustc-demangle always omit zero-valued disambiguators of crate-root paths. There is an accompanying PR to the symbol mangling documentation, which more explicitly recommends this behavior and gives some rationale for it. However, the behavior is already in line with existing documentation. E.g. the original RFC states that zero-disambiguators can always be omitted, and the current documentation also omits the disambiguators for crate-root.

Being able to rely on C3foo to be rendered as foo (i.e. without explicit disambiguator value) rather than as foo[0] allows the compiler to encode things like new basic types in a backward compatible way. This idea has been originally proposed by @eddyb in rust-lang/rust#122106. It is a generally useful mechanism for supporting a certain class of new elements in the v0 mangling scheme in a backward compatible way (whether as a temporary workaround until downstream tooling has picked up grammar changes or as a permanent encoding).

cc @tgross35

src/v0.rs Outdated Show resolved Hide resolved
#[test]
fn demangle_crate_with_zero_disambiguator() {
t!("_RC4f128", "f128");
t_nohash!("_RC4f128", "f128");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these tests fail without the change? When I was trying to work on this it seemed like these simple tests wouldn't emit [0] even as-is, and I was struggling to write a test that replicates the behavior from rust-lang/rust#123816 (comment). Hence my messy branch at https://github.com/tgross35/rustc-demangle/blob/b29eead5e3337030843b978736748a3f1c142416/src/v0.rs#L1284-L1295

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The t! line should fail, the other one should already pass.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified again that it fails without the change:

thread 'v0::tests::demangle_crate_with_zero_disambiguator' panicked at src/v0.rs:1286:9:
assertion `left == right` failed
  left: "f128[0]"
 right: "f128"

@eddyb eddyb enabled auto-merge April 29, 2024 13:46
@eddyb
Copy link
Member

eddyb commented Apr 29, 2024

image

Looks like there's some misconfiguring, the required "test" isn't one of the the CI checks in this repo.

EDIT: I can't even manually merge, and don't have access to settings I don't think.

@eddyb eddyb disabled auto-merge April 29, 2024 13:48
@michaelwoerister michaelwoerister merged commit 0ad7b13 into rust-lang:main Apr 29, 2024
6 checks passed
@michaelwoerister
Copy link
Member Author

It seems to have worked for me. This is the setup, so everyone on the compiler and contributors should be able to merge PRs: https://github.com/rust-lang/team/blob/master/repos/rust-lang/rustc-demangle.toml

Thanks for the review, @eddyb!

@tgross35
Copy link

tgross35 commented May 1, 2024

The merging bug was an issue with branch protection by the way https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/.22test.22.20job.20not.20completing

We will need a new release for this to use it in rust-lang/rust#123816, @Mark-Simulacrum it looks like maybe you handle this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants