Remove invalid help diagnostics for const pointer by chenyukang · Pull Request #127675 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation27 Commits1 Checks6 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

chenyukang

@rustbot

r? @fee1-dead

rustbot has assigned @fee1-dead.
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 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.

labels

Jul 13, 2024

chenyukang

RalfJung

@fee1-dead

I have edited the PR description so that merging this PR does not close that issue. The best diagnostic improvement in that case would be suggesting addr_of! to be changed to addr_of_mut!.

@bors r+ rollup

@fee1-dead

@bors

📌 Commit b44a484 has been approved by fee1-dead

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

Jul 15, 2024

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Jul 15, 2024

@matthiaskrgr

… r=fee1-dead

Remove invalid help diagnostics for const pointer

Partially addresses rust-lang#127562

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jul 15, 2024

@bors

…iaskrgr

Rollup of 5 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

@RalfJung

@fee1-dead

@bors 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

Jul 15, 2024

@chenyukang

@RalfJung

It is also quite concerning that the ui test suite produces different results than playground... that's a disaster for testability of this bug. Is there an issue tracking that mismatch? Any idea how it can be fixed?

@chenyukang

Seems the document for this is at here:
https://github.com/rust-lang/rustc-dev-guide/blob/d6e3a32a557db5902e714604def8015d6bb7e0f7/src/tests/ui.md#L114

The directory to the standard library source is replaced with $SRC_DIR. Example: /path/to/rust/library
Line and column numbers for paths in $SRC_DIR are replaced with LL:COL.
This helps ensure that changes to the layout of the standard library do not cause widespread changes to the .stderr files. Example: $SRC_DIR/alloc/src/sync.rs:53:46

so when the code logic is base on something like tcx.sess.source_map().span_to_snippet(span), it will return an Err:

Err(SourceNotAvailable { filename: Real(Remapped { local_path: None, virtual_name: "$SRC_DIR/core/src/ptr/mod.rs" }) })

then the code flow changes...

@chenyukang

@chenyukang

@RalfJung

Seems the document for this is at here: https://github.com/rust-lang/rustc-dev-guide/blob/d6e3a32a557db5902e714604def8015d6bb7e0f7/src/tests/ui.md#L114

The directory to the standard library source is replaced with $SRC_DIR. Example: /path/to/rust/library
Line and column numbers for paths in $SRC_DIR are replaced with LL:COL.
This helps ensure that changes to the layout of the standard library do not cause widespread changes to the .stderr files. Example: $SRC_DIR/alloc/src/sync.rs:53:46

so when the code logic is base on something like tcx.sess.source_map().span_to_snippet(span), it will return an Err:

Err(SourceNotAvailable { filename: Real(Remapped { local_path: None, virtual_name: "$SRC_DIR/core/src/ptr/mod.rs" }) })

then the code flow changes...

No, that just does some post-processing of the output, so that can't be it.

@oli-obk do you understand what is going on here? We should definitely have an issue about this -- when I copy some code from the playground to a ui test, it should behave the same, otherwise we aren't even testing the thing that people will be using in the wild!

@oli-obk

idk, we do try to actually do what happens outside of the test suite, but there are some corner cases around libstd that are not easy and no one felt like it's too important to actually work on it.

@alex-semenyuk alex-semenyuk 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-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Oct 11, 2024

@alex-semenyuk

@chenyukang
From wg-triage. Could you please clarify the status of PR?

@chenyukang

I think we could merge this PR first and create a new issue to track the test suite issue in #127675 (comment) ?

@chenyukang

@fee1-dead

RalfJung

sugg,
Applicability::MachineApplicable,
);
if sugg.iter().all(|(span, _)

Choose a reason for hiding this comment

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

The source of the problem here is that the suggestion points inside a macro. This is IMO not something that should be fixed in this specific diagnostic, it affects all daignostics and thus needs a central fix.

I don't know the diagnostic system well enough though to suggest how this could be fixed.
Cc @rust-lang/wg-diagnostics

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Hm, that's not what this code does though?

@RalfJung

It is also quite concerning that the ui test suite produces different results than playground... that's a disaster for testability of this bug. Is there an issue tracking that mismatch? Any idea how it can be fixed?

I filed an issue for that: #131782

@petrochenkov

@bors

📌 Commit 7ff71e5 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

Oct 19, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Oct 19, 2024

@bors

…iaskrgr

Rollup of 12 pull requests

Successful merges:

Failed merges:

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Oct 20, 2024

@rust-timer

Rollup merge of rust-lang#127675 - chenyukang:yukang-fix-127562-addr, r=petrochenkov

Remove invalid help diagnostics for const pointer

Partially addresses rust-lang#127562

@cyrgani

This PR does not even fully solve the invalid diagnostic problem though: while addr_of! no longer suggests &mut raw const val, expanding the macro (which is recommended) does:

fn main() { let val = 2; let ptr = &raw const val; unsafe { *ptr = 3; } }

Error:

error[E0594]: cannot assign to `*ptr`, which is behind a `*const` pointer
 --> src/main.rs:4:14
  |
4 |     unsafe { *ptr = 3; }
  |              ^^^^^^^^ `ptr` is a `*const` pointer, so the data it refers to cannot be written
  |
help: consider changing this to be a mutable pointer
  |
3 |     let ptr = &mut raw const val;
  |                +++

For more information about this error, try `rustc --explain E0594`.

@chenyukang

This PR does not even fully solve the invalid diagnostic problem though: while addr_of! no longer suggests &mut raw const val, expanding the macro (which is recommended) does:

fn main() { let val = 2; let ptr = &raw const val; unsafe { *ptr = 3; } }

Error:

error[E0594]: cannot assign to `*ptr`, which is behind a `*const` pointer
--> src/main.rs:4:14
 |
4 |     unsafe { *ptr = 3; }
 |              ^^^^^^^^ `ptr` is a `*const` pointer, so the data it refers to cannot be written
 |
help: consider changing this to be a mutable pointer
 |
3 |     let ptr = &mut raw const val;
 |                +++

For more information about this error, try `rustc --explain E0594`.

Yes, the original issue is not closed, this PR only remove this part, we should not suggest code change for any span which is not written by user:

help: consider changing this to be a mutable pointer --> /home/user/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:2189:6 | 21| &mut raw const $place | +++

Labels

A-diagnostics

Area: Messages for errors, warnings, and lints

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.