Skip to content
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 lints: functions with explicit ABI export names #502

Open
5 tasks
obi1kenobi opened this issue Jul 21, 2023 · 2 comments
Open
5 tasks

New lints: functions with explicit ABI export names #502

obi1kenobi opened this issue Jul 21, 2023 · 2 comments
Labels
A-lint Area: new or existing lint E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue.

Comments

@obi1kenobi
Copy link
Owner

obi1kenobi commented Jul 21, 2023

Consider a function like this:

#[no_mangle]
extern "C" fn example() {
    // does stuff
}

The following change is non-breaking, since it's equivalent:

#[export_name = "example"]  // matches original function name
extern "C" fn renamed() {  // this function name doesn't matter anymore
    // does stuff
}

However, changing to any other export_name value (one that does not match the original name of the function) is breaking.

Example code where you can play with #[no_mangle] and #[export_name]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=64663d9ca343dee3ba9d2c7cabe61ab4

We have an export_name property on functions which sorts out #[no_mangle] and #[export_name] under the hood, and presents the actual export name (if any) of the function: https://github.com/obi1kenobi/trustfall-rustdoc-adapter/blob/rustdoc-v27/src/rustdoc_schema.graphql#L618

We want the following lints:

  • function that was previously exported no longer has an export name
  • function's export name has changed
  • function with the same export name takes different number of arguments
  • function with the same export name has a parameter whose type changes to one with a different ABI
    • it doesn't matter what the type is called, just whether it has the same ABI or not
  • function with the same export name changes what it returns (starts / stops returning data, or changes to return type with different ABI)
    • this can be multiple lints; e.g. separate lints for starts / stops / changes return type ABI
@obi1kenobi obi1kenobi added A-lint Area: new or existing lint E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue. labels Jul 21, 2023
@suaviloquence
Copy link
Contributor

Hi! I started to work on this. A couple of questions/uncertainties:

  1. I'm assuming it's a major-level change based on other violations like changing an established repr, although I couldn't find anything on the cargo semver page. for no_mangle/export_name specifically
  2. Is it a violation when a non-public function changes export name? For example, in the example you linked, you can still access im_secretly_public even when it's not public in the Rust API.
  3. Should this (these) lints proc when the export name of one function moves to a different function? For example,
#[no_mangle]
pub fn func1() {}

pub fn func2() {}

to

pub fn func1() {}

#[export_name = "func1"]
pub fn func2() {}

Thanks!

@obi1kenobi
Copy link
Owner Author

obi1kenobi commented Mar 28, 2024

Great questions!

I'm assuming it's a major-level change based on other violations like changing an established repr, although I couldn't find anything on the cargo semver page. for no_mangle/export_name specifically

Yes, it's a major breaking change. The cargo semver reference isn't yet an exhaustive list of major changes, and we've had many situations where cargo-semver-checks work triggered additions to the reference or vice versa. I expect that trend will continue for some time :)

Is it a violation when a non-public function changes export name? For example, in the example you linked, you can still access im_secretly_public even when it's not public in the Rust API.

Yes! I believe we might actually have a lint for this already. It seems familiar and may have been added sometime in the last couple of months.

Should this (these) lints proc when the export name of one function moves to a different function? For example,

Great edge case! No, this should not trigger either lint.

In general, we want to trigger lints when we are certain there's a problem, so that we skew heavily away from false-positives which cause users to distrust the tool. In this edge case, the change may have been intentional and from an API/ABI perspective seems fine.

However, if the func2() function e.g. takes a different number of arguments, then that should trigger a lint — I'm adding that plus related cases to the list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: new or existing lint E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue.
Projects
None yet
Development

No branches or pull requests

2 participants