Remove nondeterminism in multiple-definitions test

[ricobbe]

Compare all fields in DllImport when sorting to avoid nondeterminism in the error for multiple inconsistent definitions of an extern function. Restore the multiple-definitions test.

Resolves #87084.

[rust-highfive]

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

[ricobbe]

r? @petrochenkov

[ricobbe]

Question for reviewers: in an earlier version of this code, a reviewer asked me not to have DllImport implement PartialOrd or Ord, I think because the ordering relation as implemented then didn't consider all of DllImport's fields, but it's been a while and I don't remember for certain. The new ordering relation I'm defining here does indeed consider all of the fields, and it's effectively equivalent to the one that we'd get if I just derived PartialOrd and Ord. Would you prefer me to just do that instead?

[fee1-dead]

I don't think it is nondeterministic when you compared the names, so I don't think the issue is that simple.

[ricobbe]

I don't think it is nondeterministic when you compared the names, so I don't think the issue is that simple.

The error message is triggered in precisely the case where we have multiple DllImports with the same name. Therefore, to get a unique, well-defined result from sorting the imports (and therefore selecting the second entry with a duplicate name for the error message), I need to compare based on more than just the names.

[petrochenkov]

My version of the fix, with all unnecessary impls removed, and with explicit sorting removed too - petrochenkov@8b9ce0e

(Not tested on src/test/ui/rfc-2627-raw-dylib/multiple-definitions.rs because I don't have a readily available MSVC setup.)

[ricobbe]

My version of the fix, with all unnecessary impls removed, and with explicit sorting removed too - petrochenkov@8b9ce0e

I'm happy to just go with that; what's the workflow to do that?

[petrochenkov]

@ricobbe
You can cherry-pick that commit and add/update the test.

[ricobbe]

@petrochenkov

You can cherry-pick that commit and add/update the test.

Ok, I've tried that, and now the restored (and renamed) multiple-declarations.rs test is failing. I've run it with --bless and updated the expected error message (in the comment in the .rs file) to adjust for the change in wording, but the test still fails:

error: process did not return an error status
status: exit code: 0

Everything else looks reasonable; in particular, I'm seeing the expected diagnostics on stderr.

I've tried running the test with the // check-pass header command, and also with // build-pass, but neither have any effect on the results. This is not a failure mode I know how to debug, I'm afraid.

EDIT: with @wesleywiser's help, I was able to figure out why rustc was terminating with an exit code of 0 for this test despite the error message; a little bit of plumbing sufficed to get this fixed up. I'll push shortly.

[petrochenkov]

r=me after addressing #87092 (comment) and squashing commits.

[ricobbe]

@rustbot label -S-waiting-on-author +S-waiting-on-review

[wesleywiser]

@bors r=petrochenkov rollup

[bors]

📌 Commit ce59f1a has been approved by petrochenkov