Deny clashing extern declarations by default.

[jumbatm]

Changes the clashing_extern_declarations lint to be deny by default, as the lint addresses a soundness issue (see #70496).

Note that this change should go through a crater-check run before being merged (as mentioned in #70946 (comment)).

r? @nagisa

[joshtriplett]

Looks good to me.

@bors try

[bors]

⌛ Trying commit 40db56c with merge d85ecff64d4ec32056a636d93cc8427a25ef742d...

[bors]

⌛ Trying commit 40db56c with merge 169954edd744527414c631e9af1bbdc537b0cdf2...

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: 169954edd744527414c631e9af1bbdc537b0cdf2 (169954edd744527414c631e9af1bbdc537b0cdf2)

[joshtriplett]

@craterbot check

[craterbot]

👌 Experiment pr-75192 created and queued.
🤖 Automatically detected try build 169954edd744527414c631e9af1bbdc537b0cdf2
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-75192 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-75192 is completed!
📊 11 regressed and 4 fixed (116161 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[nagisa]

Looks like there may be some genuine regressions in there (most notably about crates using a (same? didn’t check) structure as an argument, where the structure is defined twice. Some others are genuine problems in the crates themselves. Some others are signal 9s (probably OOMs or out-of-times).

[jumbatm]

I took a closer look at the regressed (by clashing_extern_declarations) crates. As a summary:

• https://crater-reports.s3.amazonaws.com/pr-75192/try%23169954edd744527414c631e9af1bbdc537b0cdf2/gh/1000VICKY.ishell_in_Rust/log.txt -- redeclares printf in various, inconsistent ways. I believe the lint is correct -- the correct, consistent signature should be with varargs.
• https://crater-reports.s3.amazonaws.com/pr-75192/try%23169954edd744527414c631e9af1bbdc537b0cdf2/gh/rybot666.bootloader/log.txt is an interesting case. There's a clash in the function signature _improper_ctypes_check which isn't used anywhere; it looks like the function exists to check certain structures for improper_ctypes that aren't otherwise used locally in any extern C functions.
  • Ditto for https://crater-reports.s3.amazonaws.com/pr-75192/try%23169954edd744527414c631e9af1bbdc537b0cdf2/reg/os_bootinfo-0.2.1/log.txt
• https://crater-reports.s3.amazonaws.com/pr-75192/try%23169954edd744527414c631e9af1bbdc537b0cdf2/reg/awi-0.8.0/log.txt redeclares free for a number of structures, instead of consistently declaring it with a *const c_void parameter.
• https://crater-reports.s3.amazonaws.com/pr-75192/try%23169954edd744527414c631e9af1bbdc537b0cdf2/reg/gli-rs-0.4.0/log.txt-- the clash appears in generated code, and appears to be by-design -- the external function must do some kind of runtime typechecking that makes the call safe
• https://crater-reports.s3.amazonaws.com/pr-75192/try%23169954edd744527414c631e9af1bbdc537b0cdf2/reg/os_bootinfo-0.2.1/log.txt is an error on the same code as in rybot666.bootloader
• https://crater-reports.s3.amazonaws.com/pr-75192/try%23169954edd744527414c631e9af1bbdc537b0cdf2/reg/vrust-0.0.1/log.txt is for a clash between an empty type enum xcb_connection_t { } and zero-sized type struct xcb_connection_t { [0; u32]; } that are both used opaquely. From what I understand, there's no memory unsafety here, but they're semantically not quite the same because the zst can be instantiated, while the empty type cannot. Again, appears in generated code.

Given these results, and some further reflection, I don't think we should proceed with making clashing_extern_declarations deny because

• It would break builds using generated code (until tools like bindgen were updated, anyway)
• Without knowing the behaviour of the external C code, I feel like we don't really have all the information we need to warrant erroring out the build -- gli being an example of this

Thinking about this more, I also realise the user doesn't really get any more value out of making the lint deny-by-default: as a warning, the lint already pushes the user to fix mistakes in the extern function declaration or allow the clash if they know it's safe.

For these reasons, I'm going to close this PR. Feel free to re-open -- I'm happy to discuss further.