New lints: fn changed ABI

[obi1kenobi]

Lints:

• [x] pub fn changed ABI to an ABI of a different name, regardless of unwind: e.g. "C" to "Rust", or "system-unwind" to "C"
  • this changes the calling convention for the function, which is a breaking change for external code that depends on it
• [x] pub fn changed from an unwind-capable ABI to the same-named ABI without unwind: "C-unwind" to "C" (#689)
  • this makes unwinding become UB, which is a breaking change
• [ ] pub fn changed from a non-unwind ABI to the unwind-capable equivalent of that same ABI, e.g. "C" to "C-unwind"
  • In principle, this could be combined with the lint above, but let's not do that. Some projects may wish to enable only one of these two lints and not the other, as unwind-capable ABIs were introduced relatively recently.
• [x] fn marked #[no_mangle] or #[export_name] with no importable names changed to ABI of different name
  • no importable names = it's either private or pub-in-priv -- this disambiguates the lint from the pub fn lints
  • we have no importable names to match on, but #[no_mangle] or #[export_name] means the function's export name must be globally unique, so match on that
  • we can use the export_name: String property on Function (implemented here), to simplify matching and account for both #[no_mangle] and #[export_name]

[obi1kenobi]

The third checkbox was handled in #650.

[qstommyshu]

Hi, I would like to try the remaining task on this issue. I tried to understand the pattern from the PR for the two completed task from this issue, but there are still things I don't understand, like how to check what are the fields available to a baseline and when and why do we choose to write ... on Function {} at some point. Can you please give me some guidance on these? Thanks!

[obi1kenobi]

Thanks for looking into this.

Have you had a chance to check out the schema these queries are written against, or tried out some queries in the rustdoc query playground? If not, they are good to take a peek at. Note that the playground can only query one crate at a time — it can't compare two different versions at once.

The baseline edge points to type Crate, and Crate's item edge points to interface Item. We aren't looking for just about any Item, though, we want Function specifically — hence ... on Function.

Hope this helps! Let me know if you have any more questions!

[qstommyshu]

Thanks for the guide! I think I start to have an idea on how to add a new lint now. A little issue came up when I try to add a lint.

I followed the contribution guide to add a new lint. However, it failed along with many other lints with the error code: "Once instance has previously been poisoned"

Is there a fix for these lints? Many of them fails even in a freshly cloned repo.

[obi1kenobi]

That error message means there was another, earlier error that caused the Once data structure to not be set up correctly. Scroll up in the log and you should see the underlying error info.

If they fail in a freshly cloned repo, then my best guess is that you might not have the right version of Rust installed.

[qstommyshu]

Got it, thanks. It turns out the cause of my issue was I didn't generate the required rust doc JSON before running cargo test. I've fixed it and added a new lint in #683. Can you please review it when you have time? Thanks!

[pksunkara]

@obi1kenobi The second & third points in this are done, right? I am asking because they not ticked.

[obi1kenobi]

Yeah! So I guess this issue seems done -- nice!

[pksunkara]

I think I got confused yesterday. I haven't seen the 3rd one implemented. Can you please double check?

[obi1kenobi]

Agreed, not done yet. Reopening.

We probably want to do a review of the lints and test cases here, to ensure they are consistent and have minimal overlap. It looks like the lint that matches on #[export_name] uses the raw ABI string (e.g. "C-unwind") to compare, meaning it doesn't offer more granular feedback on the nature of the breakage: whether it's unwinding-related only, or something more.

It's also likely that if a function is both #[export_name] / #[no_mangle] and also public API, we might get duplicate lints specifically about ABI breakage. We should think through how we structure the lints (and write appropriate test cases) to do our best to avoid this.

[pksunkara]

Yup, agreed. That is one of the reasons I got super confused yesterday when looking at this group.