Closed
Conversation
During local development it doesn't matter, for end users it doesn't help as it doesn't affect the precompiled rustc executable and during development of custom drivers, most paths are kept remapped anyway due to rustc not getting recompiled. As such for the use cases where you want the original source paths, you either already have the original source paths or un-remapping doesn't have any effect.
rustbot
added
S-waiting-on-review
Collaborator
|
r? @chenyukang rustbot has assigned @chenyukang. Use Why was this reviewer chosen?The reviewer was selected based on:
|
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Member
|
This un-remapping was added specifically for Clippy needs, and does have an effect, see #142377. cc @samueltardieu |
Member
|
The original discussion was in this Zulip topic. |
Urgau
added
S-waiting-on-author
Member
Author
|
I don't understand what the issue there was. Why does it matter for clippy that source paths are mapped back to an on-disk file? The debuginfo will have many paths not un-remapped anyway and for diagnostics errors in clippy shouldn't generally result in rustc source being used. |
Member
|
For example, some Clippy lint messages point to the snippet where an entity was defined. Since Clippy itself uses types, functions, and macros from the compiler libraries, when linting Clippy it will show code snippets coming from the compiler sources. Any program using the compiler libraries would suffer from the same problem when linted with Clippy, or more generally when attempting to retrieve the source snippet corresponding to some place in the compiler libraries, or even to point at the file on disk where this code comes from. |
Member
Author
Is that actually important? The source of the error is in clippy, not in rustc. |
Member
Well, for the quality of error messages, it is. And with no or improper remapping, we hit a situation where retrieving a snippet doesn't work, which never happens otherwise. You didn't tell why it was important to you to remove this un-remapping? How does it hurt? Note that the un-remapping hasn't been added for Clippy, it has been fixed for Clippy, as you can see in the original issue, it was incorrect before. |
Member
Author
Does it actually matter that retrieving the snippets doesn't work? It is already the case for most people that the standard library source is unavailable, so rustc has to (and should be) able to generate high quality diagnostics when the sources for a dependency are missing.
Un-remapping was incorrectly applied but #141751 fixed that to not try to un-remap at all. #142377 then made it remap to the correct location. This PR only reverts #142377.
It doesn't hurt a lot. I'm just not that happy with the un-remapping. It doesn't just affect diagnostics, but also causes un-remapped paths to end up in the debuginfo depending on if the source files are available or not, which both hurts reproducibility and AFAIK isn't tracked by cargo (so cargo incorrectly doesn't rebuild when you install rust-src, causing a mix between crates where un-remapping is applied and ones where it isn't). I figured I could remove it for rustc sources to make it a tiny bit less of an issue. But if you feel strongly against it, I don't mind closing this PR. |
Member
|
If you don't mind, I would prefer indeed to postpone this PR: I would like to evaluate the consequences of not getting some snippets. This is handled of course, in the sense it won't crash, but I suspect that we have many places where the way to handle this is akin I'll add this to my TODO list. |
rustbot
removed
the
S-waiting-on-author
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
During local development it doesn't matter, for end users it doesn't help as it doesn't affect the precompiled rustc executable and during development of custom drivers, most paths are kept remapped anyway due to rustc not getting recompiled. As such for the use cases where you want the original source paths, you either already have the original source paths or un-remapping doesn't have any effect.