add function_export_name_changed lint #728

suaviloquence · 2024-03-30T18:39:52Z

Second bullet point of #502. Should I put all the lints in one (draft, I haven't finished them yet) PR or do separate PRs?

suaviloquence · 2024-03-30T18:51:34Z

FWIW here's the line that breaks nightly tests - it doesn't seem like an issue with this lint.

obi1kenobi · 2024-03-30T22:04:50Z

Separate PRs for each lint is fine. That's usually faster and easier to review, and shows up more nicely in the changelog as well.

That test has been flaky for a while and I'm not sure why. I've never managed to reproduce the flakiness locally, so my gut feeling is that it might possibly be a GitHub Actions bug of some sort? Not sure, haven't had time to dig into it... Don't worry about it, I don't think you broke any of it — I just restarted it and I expect it'll probably pass.

obi1kenobi

I might have unintentionally pointed you a bit in the wrong direction on this. Sorry about that!

It should be easy to straighten things out. This is part of why semver linting is so hard! Too many edge cases! But getting them right is important, and I feel like our users will appreciate it.

src/lints/function_export_name_changed.ron

obi1kenobi · 2024-03-30T22:12:03Z

src/lints/function_export_name_changed.ron

+                    ... on Function {
+                        name @output
+
+                        export_name @filter(op: "!=", value: ["%export_name"]) @filter(op: "!=", value: ["$null"])


This is a failure of our docs, so don't worry about it -- there's a built-in filter operator for this:

Suggested change

export_name @filter(op: "!=", value: ["%export_name"]) @filter(op: "!=", value: ["$null"])

export_name @filter(op: "!=", value: ["%export_name"]) @filter(op: "is_not_null")

obi1kenobi · 2024-03-30T22:12:24Z

src/lints/function_export_name_changed.ron

+    }"#,
+    arguments: {
+        "zero": 0,
+        "null": null,


Suggested change

"null": null,

since we used the built-in filter operator for this

obi1kenobi · 2024-03-30T22:20:35Z

src/lints/function_export_name_changed.ron

+                item @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]) {
+                    ... on Function {
+                        export_name @filter(op: "=", value: ["%export_name"])
+                    }
+                }


It might be a good idea to write the lint for "export name no longer exists" first, just to get us a bit more test coverage and help us decide how to split the responsibilities between this lint and that one.

Here's what I'm worried about.

If both this lint and that other one enforce that the export name doesn't exist anymore, then it sounds like this lint is just a stricter version of that other one -- so whenever this one triggers, the other one will too. We want to avoid that.

So maybe we reshape this lint a bit to say "a function in the public API has changed its export name in the ABI" instead. So the invariant that is broken (and that we're reporting) is "call func() via the API, and export_func() via the ABI, those do the same thing because they are the same function." If the ABI export name changes to something else, then even if the ABI name export_func() continues to exist on some other function, this is likely worth flagging because it's a possible bug.

If we do end up taking that route, then I think:

This block I've highlighted becomes unnecessary.

The other lint doesn't need to look at importable_path at all, and can just focus on export_name alone.

What do you think?

Yeah, I was thinking about that when I was writing it - it was pretty much the same query except checking if the current export_name is null. If we don't need granular feedback between missing vs changed, they can be easily combined. Do you think it's necessary?

I'm not at my computer right now but I can push the other lint to a branch when I get a chance

obi1kenobi · 2024-03-30T22:21:40Z

test_crates/function_export_name_changed/new/src/lib.rs

+/// negative test - export name on one function gets moved to the other
+/// this is not necessarily a breaking change, as long as the ABIs are the same
+/// but that is a different lint
+pub mod export_name_moved {
+    pub fn export_name_moved_1() {}
+
+    #[export_name = "export_name_moved"]
+    pub fn export_name_moved_2() {}
+}


I think we may want to flag this, because a public API call to export_name_moved::export_name_moved_1() is no longer equivalent to a call to export_name_moved() via the ABI.

See comment on the lint above for details.

obi1kenobi · 2024-03-30T23:02:35Z

Yeah, we can just the missing one for both cases, I think that's fine.

…

On Sat, Mar 30, 2024, 6:59 PM Max Carr ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/lints/function_export_name_changed.ron <#728 (comment)> : > + item @fold @Transform(op: "count") @filter(op: "=", value: ["$zero"]) { + ... on Function { + export_name @filter(op: "=", value: ["%export_name"]) + } + } Yeah, I was thinking about that when I was writing it - it was pretty much the same query except checking if the current export_name *is* null. If we don't need granular feedback between missing vs changed, they can be easily combined. Do you think it's necessary? — Reply to this email directly, view it on GitHub <#728 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAR5MSRHMSANZ27FFUX257TY247WRAVCNFSM6AAAAABFPX6VJOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNZQGA3DMOJVGU> . You are receiving this because you commented.Message ID: ***@***.***>

suaviloquence · 2024-03-30T23:06:42Z

All good. For these categories from the original issue:

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

Would these (just) apply to exported non-public functions? Because changing the API of a public function would already be a breaking change, so do we want to flag this twice?

obi1kenobi · 2024-03-31T06:43:58Z

That's a great question.

I think it's reasonable to start by flagging twice here, because we're flagging two separate problems: the API is broken, and the ABI is also broken.

Down the line, if this proves to be problematic for any reason, we can build something more sophisticated. For example, we could have three lints instead of two: broken API but not ABI, broken ABI but not API, and both broken at once. But going straight for this seems like major overkill right now, so let's go with the simpler option of flagging twice for now.

suaviloquence · 2024-03-31T18:49:47Z

OK, I consolidated the two cases

obi1kenobi

Just a set of tiny nitpicks. The lint is correct and the tests are in great shape as well. Less than 5min of polishing left and this is good to merge.

obi1kenobi · 2024-03-31T23:49:19Z

src/lints/function_export_name_changed.ron

+    required_update: Major,  // Not specified in rust semver doc? most similar is for #[repr] changes: https://doc.rust-lang.org/cargo/reference/semver.html#type-layout
+    reference_link: Some("https://doc.rust-lang.org/reference/abi.html#the-no_mangle-attribute"),  // TODO


Did you mean to leave both of these comments in the file that will get merged?

Asking because the TODO seems completed, and the other comment seemed like a question for reviewers that I believe I replied to in my previous comment near here.

src/lints/function_export_name_changed.ron

test_crates/function_export_name_changed/new/src/lib.rs

test_crates/function_export_name_changed/old/src/lib.rs

obi1kenobi · 2024-03-31T23:53:01Z

test_crates/function_export_name_changed/old/src/lib.rs

+/// positive test - export name on one function gets moved to the other
+/// this is still a breaking change because a call to `export_name_moved_1` in the API
+/// is no longer equivalent to a call to `export_name_moved` in the ABI


Great comments throughout, and this one is particularly good! Well done!

src/lints/function_export_name_changed.ron

Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>

- remove old comments - add output from new `new_export_name` field

suaviloquence · 2024-04-01T00:29:35Z

Thanks for the feedback!

obi1kenobi

Awesome job!

add function_export_name_changed lint

f262c6e

obi1kenobi reviewed Mar 30, 2024

View reviewed changes

suaviloquence added 2 commits March 31, 2024 10:34

consolidate export_name_changed and export_name_missing into one lint

6267ed2

Merge branch 'main' into export_name

c8528d6

obi1kenobi reviewed Mar 31, 2024

View reviewed changes

suaviloquence and others added 2 commits March 31, 2024 17:18

Apply suggestions from code review (pt1)

eef2010

Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>

apply feedback from code review (pt2)

20975ea

- remove old comments - add output from new `new_export_name` field

obi1kenobi approved these changes Apr 1, 2024

View reviewed changes

obi1kenobi merged commit 1a48764 into obi1kenobi:main Apr 1, 2024
33 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add function_export_name_changed lint #728

add function_export_name_changed lint #728

suaviloquence commented Mar 30, 2024 •

edited

Loading

suaviloquence commented Mar 30, 2024

obi1kenobi commented Mar 30, 2024

obi1kenobi left a comment

obi1kenobi Mar 30, 2024

obi1kenobi Mar 30, 2024

obi1kenobi Mar 30, 2024

suaviloquence Mar 30, 2024

suaviloquence Mar 30, 2024

obi1kenobi Mar 30, 2024

obi1kenobi commented Mar 30, 2024 via email

suaviloquence commented Mar 30, 2024

obi1kenobi commented Mar 31, 2024

suaviloquence commented Mar 31, 2024

obi1kenobi left a comment

obi1kenobi Mar 31, 2024

obi1kenobi Mar 31, 2024

suaviloquence commented Apr 1, 2024

obi1kenobi left a comment

	export_name @filter(op: "!=", value: ["%export_name"]) @filter(op: "!=", value: ["$null"])
	export_name @filter(op: "!=", value: ["%export_name"]) @filter(op: "is_not_null")

		required_update: Major, // Not specified in rust semver doc? most similar is for #[repr] changes: https://doc.rust-lang.org/cargo/reference/semver.html#type-layout
		reference_link: Some("https://doc.rust-lang.org/reference/abi.html#the-no_mangle-attribute"), // TODO

add function_export_name_changed lint #728

add function_export_name_changed lint #728

Conversation

suaviloquence commented Mar 30, 2024 • edited Loading

suaviloquence commented Mar 30, 2024

obi1kenobi commented Mar 30, 2024

obi1kenobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obi1kenobi commented Mar 30, 2024 via email

suaviloquence commented Mar 30, 2024

obi1kenobi commented Mar 31, 2024

suaviloquence commented Mar 31, 2024

obi1kenobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suaviloquence commented Apr 1, 2024

obi1kenobi left a comment

Choose a reason for hiding this comment

suaviloquence commented Mar 30, 2024 •

edited

Loading