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.
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
New rustc and Cargo options to allow path sanitisation by default #3127
New rustc and Cargo options to allow path sanitisation by default #3127
Changes from 6 commits
139d7f0
bcbd131
97e4104
d8344ef
408dc50
f92a321
2bd2792
998ecf4
ba6b2d8
d790948
d33e029
0688580
aee42a6
8e33a46
0b59e5c
604dcb0
2d49c09
23394fc
e15308c
dec1901
bb079f4
7c533d3
6e45014
34d4386
785c229
a357827
5286008
8ba3510
3f59b7b
16edfb2
cbcb1df
451f163
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not really a descriptive name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
panic
is what most users care about, but it's not really accurate for those usingstd::file!()
directly or indirectly for other things. Sofile-macro
maybe?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file!()
isn't the only way to get a source location anymore (I don't know if its even still used for panics), we now also haveintrinsics::caller_location
(including stable wrappers).I don't have any good naming suggestions (maybe
intrinsics
?), but I believe it would be good to also mention the intrinsic or its wrappers explicitly in the RFC.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expansion
maybe?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this useful on it's own? It can't prevent the original path from ending up in the crate metadata without also requiring all other remappings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rust-lang/rust#88363
Maybe people won't use it on its own, but it can't be merged with other options either so it has to be there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since analyzer is popular that means nothing is stripped by default then? will it be a single option and not a mess of remap-prefix options then to strip the paths? otherwise it will be a choice of privacy/reproducibility or the usefulness of analyzer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--remap-path-prefix
is a stable option and will do the same things as documented after this RFC (i.e. remap everything everywhere). You only need--remap-path-scopes
if you want finer grained control.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While
trim-paths
has some precidence, this functionality isn't really about trimming - it's more to do with normali[sz]ation. (Just a comment, don't feel too strongly about this.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name-version
is only unique - at best - within a single registry. And for code coming from git, the specific commit id is probably important to encode, since it can change without version changes.Ideally it would be nice to encode a hash of the source file itself somewhere, but that probably doesn't fit well into this scheme. (I think Dwarf has a way to encode this, so it can be done in a case by case basis.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it should use the full package id as used by cargo? Package id's look like
libc 0.2.126 (registry+https://github.com/rust-lang/crates.io-index)
. The remapping could then be/cargo/libc 0.2.126 (registry+https://github.com/rust-lang/crates.io-index)/src/lib.rs
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading this entire section, it doesn't seem to me that this functionality needs to live in Cargo. rustc could directly provide a
-C trim-paths
option with this behavior, since AFAICT rustc has all the information needed to do so. Cargo can then just pass through trim-paths to rustc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is rustc aware of the path to the current package? This is relevant to dependencies living under
$HOME/.cargo/registry
. Currently there is also the possibility (mentioned in unresolved questions) to encode more information about the package to the sanitised path, such as registry name and git commit hashThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, fair point. I thought that rustc had the necessary information, but perhaps not.
It might make sense to pass the requisite information into rustc, but then let rustc make the decision for how to use that information. I'd rather not have cargo making that decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rustc always knows the absolute path to files, but only the build tool can tell which part of it is environment-sensitive. E.g. rustc knows it's using
/home/cbeuw/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/rand_core-0.6.3/src/lib.rs
, but the/home/cbeuw/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/rand_core-0.6.3
part is up to Cargo, and it would be different if a different registry were used, or the structure of that path may be completely different for Bazel dependencies - but rustc doesn't know that.This "requisite information" is already included in provided mappings in
--remap-path-prefix
. I can't see how a separate argument that lets the build tool pass in "the sensitive part of the current path" would look like. It'd probably be either too build tool-specific, requiring rustc to know about things like Cargo registries or Bazel repositories, or too "free", ultimately being the same as the existing--remap-path-prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would propose that, rather than using numbers here (which are not forwards-compatible if we want to add more levels in between), we should use descriptive terms:
trim-paths=none
ortrim-paths=false
(don't trim any paths)trim-paths=object
(trim paths only within the compiled object)trim-paths=all
ortrim-paths=true
(trim all paths, including diagnostics, split debuginfo files, etc)This would allow us to, in the future, add more names for useful subsets.
I'm also wondering if this could then be merged into
remap-path-scopes
; we could just add aremap-path-scopes=object
as an alias for "everything included in the object", and aremap-path-scopes=all
to include everything. That would make it convenient to write things likeremap-path-scopes=object,debuginfo
if you want to remap paths in the object and in split debuginfo but not in diagnostics.Cargo could then just offer a single option that takes a list of those scopes or aliases for combinations of scopes, and passes them through to rustc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with using strings instead of numbers.
I've added the alias idea to future possibilities. It looks convenient but I guess we could wait and see if people would like to use it. This can be implemented at anytime without backwards compatibility issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cbeuw The main reason to add it now would be to simplify Cargo's job, so that it can just pass through a value or list of values to rustc.
I'd personally prefer to have the
all
andobject
aliases as part of the initial RFC.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added
all
,true
andobject
aliases, so the option in Cargo'strim-paths
can be supplied directly to--remap-path-scope
, along with the two--remap-path-prefix
mappings. Except for whentrim-paths
isnone
orfalse
, then none of--remap-path-scope
or--remap-path-prefix
are supplied to rustc.Should we also allow
--remap-path-scope
to take onnone
andfalse
? In which case it disables all effects of--remap-path-prefix
, though in this case one should always simply not supply anything. Also it's not freely combinable with other scopes (the current ones are all additive), what happens when the user doesmacro,none
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean by "path introduced by include!() will be remapped". The path that's actually included is not subject to remapping. Do you mean a
file!()
macro in aninclude!()
ed file will be remapped? If so, yeah, I think that would be the expected behaviour. Presumably if the path is out of the scope of any of the remappings (include!("/tmp/randomcode.rs")
) then it will be left as-is.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a panic is raised inside a file that's been
include!()
ed, it's the file that directly contains the panic gets embedded and printed, not the includer. This sentence in the RFC means that the includee path is sanitisede.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is somewhat redundant. The remapping happens for the file corresponding to the source span.
include!()
puts the included file in the source span, and only keeps the file which contained theinclude!()
as macro expansion source. Other macros may make other choices.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will make it unusable with
cargo run
, see rust-lang/rust#87825 (comment)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to unresolved questions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rust-analyzer uses expect-test which does exactly this to update snapshot tests. Most of the time tests would run in debug mode I think though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test
profile is inherited fromdev
so the default behaviour ofcargo test
won't change. The line "We only need to change the behaviour for Test and Build compile modes." means other compile modes likecargo check
can simply ignore the newtrim-paths
, though the wording is a bit ambiguous.