Add forbid(unsafe_code)

[mleonhard]

Hi proc-macro2 Maintainers,
I am developing a safe regular expression library. Of its dependencies, only proc_macro2 is lacking a forbid(unsafe_code) declaration. Can we please add it?

Best,
Michael

Cargo Geiger Safety Report

Metric output format: x/y
    x = unsafe code used by the build
    y = total unsafe code found in the crate

Symbols: 
    🔒  = No `unsafe` usage found, declares #![forbid(unsafe_code)]
    ❓  = No `unsafe` usage found, missing #![forbid(unsafe_code)]
    ☢️  = `unsafe` usage found

Functions  Expressions  Impls  Traits  Methods  Dependency

0/0        0/0          0/0    0/0     0/0      🔒  safe-regex 0.1.0
0/0        0/0          0/0    0/0     0/0      🔒  └── safe-regex-macro 0.1.0
0/0        0/0          0/0    0/0     0/0      ❓      ├── proc-macro2 1.0.24
0/0        0/0          0/0    0/0     0/0      🔒      │   └── unicode-xid 0.2.1
0/0        0/0          0/0    0/0     0/0      🔒      └── safe-regex-compiler 0.1.0
0/0        0/0          0/0    0/0     0/0      ❓          ├── proc-macro2 1.0.24
0/0        0/0          0/0    0/0     0/0      🔒          └── quote 1.0.8
0/0        0/0          0/0    0/0     0/0      ❓              └── proc-macro2 1.0.24

0/0        0/0          0/0    0/0     0/0    

[dtolnay]

Closing as a duplicate of #261.

[ralpha]

I support @mleonhard in this.
Adding this change does not subtract anything from this crate, it only adds value.
I think blocking this change because @dtolnay states "It isn't really a 'policy'." is not the right decision for the community.
I don't know why @dtolnay is against this. As other crates you created pride themself on being safe.

There is a place and time for unsafe code, I agree with that. But that is not what we are talking about. There is no unsafe code here. And thus adding the #![forbid(unsafe_code)] flag helps linting tools out.

I think we should invite others to state there opinion so we take more peoples opinion into account. (@alexcrichton )
And leaving this PR open will invite more people to comment.

@mleonhard I would like to suggest to add #![forbid(unsafe_code)] to the build.rs too . This way cargo geiger correctly marks the crate as safe.

Because the alternative is a fork of this crate, but doing that over such a small issue is hardly what we want. This divides and pollutes the package manager. This will be a loss for the the rust community if things are always forked over discussions.
@dtolnay I really like what you are doing for the rust community, but please, this is such a fundamental create for the community marking it as safe will safe so many peoples time in the future.
Thank you for considering this!

[taiki-e]

I would like to suggest to add #![forbid(unsafe_code)] to the build.rs too . This way cargo geiger correctly marks the crate as safe.

Note that it is a cargo-geiger bug that cargo-geiger requests forbid(unsafe_code) in build.rs: geiger-rs/cargo-geiger#116

Build scripts and proc-macros have a totally different attack model than normal code, they can run or insert arbitrary code at build time. (Even if the macro itself does not contain unsafe code, it does not mean that the macro is safe.)

(I feel the behavior of cargo-geiger that not ignoring proc-macro and its dependencies is also actually like a bug.)

[ralpha]

If this will be solved in cargo geiger, that is perfect! Then there is no need for the build.rs change.

I don't know what you mean by:

(I feel the behavior of cargo-geiger that not ignoring proc-macro and its dependencies is also actually like a bug.)

Could you clarify?

[dtolnay]

Adding this change does not subtract anything from this crate, it only adds value.

Adding this change involves misleading people about the safe code posture of this crate, and my belief is that it's a net negative in the long term.

Proc-macro2 has used some unsafe code in a previous version. It went away at some point not because we became opposed to unsafe code, but purely because the public API it supported was no longer needed. It's reasonably likely that a future API addition will cause this crate to use unsafe code once again in a future version.

The thing that will happen if we put forbid(unsafe_code) is that we later add a piece of OBVIOUSLY CORRECT unsafe code as part of a future feature, and people come crying that they thought they were promised safety, and that how could we, this is now the one piece of unsafe code in their dep graph, and that can't we just revert the feature please. To be clear I will always 100% welcome scrutiny over whether unsafe code we add is correct, but driving for the removal of correct necessary unsafe code is misguided as to the role of unsafe code in Rust.

I think blocking this change because @dtolnay states "It isn't really a 'policy'." is not the right decision for the community. Because the alternative is a fork of this crate, but doing that over such a small issue is hardly what we want.

I do actually want that. There is a set of safe code zealots for whom a crate having forbid(unsafe_code) is a more important priority than a crate being well maintained by competent people, and I think it is in both their interests and my interests as a maintainer if those people use a different crate.

Denying a forbid(unsafe_code) attribute is I've found a highly effective way to partition for those users for whom being well maintained by competent people is the higher priority, and that leads to a better outcome for the crate long term. I don't personally feel a responsibility to cater to both groups.

There is a place and time for unsafe code, I agree with that. But that is not what we are talking about. There is no unsafe code here.

I agree with you that it's a nuanced argument against the attribute. If it's confusing for people that we have no current unsafe code in the crate but also no forbid(unsafe_code) attribute, my preference would be to add a line of (dead) unsafe code now rather than the attribute. I would accept a PR to do that.

adding the forbid(unsafe_code) flag helps linting tools out.

Geiger is using a dark pattern to mislead people that it does. Meanwhile this crate uses the standard library heavily, including things like Vec with tons of unsafe code, much of it also written or reviewed by Alex or myself just like this crate. Any auditing burden from the code in this crate, which does not use unsafe code, is totally negligible compared to auditing the parts of the standard library we rely on.

[alexcrichton]

My personal opinion on this is that I agree with everything @dtolnay says, and I don't have anything to add to it over what's already been said.

[ralpha]

@dtolnay You make some very good arguments in your post, I like it.

I'm not entirely sure what would be the misleading part about "Adding this change involves misleading people about the safe code posture of this crate". Does it not mean that everything is typechecked, borrow checked, ... by the compiler? I think it does.
And yes, the work "safe" can be misleading. A keylogger could use "safe" code, but not be safe in the security and malware context. Is that what you mean or do I have this wrong?

Had not considered the future adding of unsafe code and the removal of the tag. I hope it is not the case that unsafe is needed, as people can always make mistakes. And unsafe is there to limit the amount of mistakes. (it is there for a reason)

Let me be clear, I have no intention to fork this repo, add the forbid(unsafe_code) flag and maintain it (at this point). But other people can. However in the scheme of things unmaintained packages are more dangerous then this.

my preference would be to add a line of (dead) unsafe code now rather than the attribute. I would accept a PR to do that.

Well I would not go that far. Then people will actively come and check the create if unsafe was used correctly and will open Issues because it is dead code. So that just moves the problem.

Geiger is using a dark pattern to mislead people that it does.

Yeah maybe, have not made my mind up over that. I think it is an interesting tool. I have checked other creates out that use extensive use of unsafe to see what is up with them. And I think having no unsafe code in something like png is a good thing. As there C counterparts are infamous for there buffer overflows and other memory bugs. (And yes dependencies are important too.)
The unsafe keyword in Rust forces people to think about there code, which is a good very thing. (hence why Rust is great, and its other features of course)

Yes, I hope the standard library is well checked. But the problems are most likely not in the standard library but in the 55,387 crate on cargo.

Thanks for you posting and very nice answer! You made a strong case, and has changes my view a bit.
I would like to see what @mleonhard thinks.

[anderejd]

Geiger is using a dark pattern to mislead people that it does.

@dtolnay If you have suggestions on rewording the readme file please do send a PR to fix it, there is no intention to "mislead people".

Meanwhile this crate uses the standard library heavily, including things like Vec with tons of unsafe code, much of it also written or reviewed by Alex or myself just like this crate. Any auditing burden from the code in this crate, which does not use unsafe code, is totally negligible compared to auditing the parts of the standard library we rely on.

Everyone who choose to use Rust also knowingly or unknowingly choose to trust the standard library and the compiler, with the exception of no_std projects. Comparing the trustworthiness of libraries on crates.io to the standard library seems like a stretch to me even when the library in question is written by well known authors who have been or are involved with the standard library.

I stumbled across this thread and I hope you don't feel annoyed by my reply and I hope to see a PR fort that readme.