Adds lint for public function ABI change #546

samuelrayment · 2023-10-03T18:15:34Z

Adds a new lint for extern function ABI changes.

I've left the unwind cases for now (this can be the next lint). If you'd like any more examples please let me know. This hopefully closes the first bullet point on this issue: #503

obi1kenobi

This is in very good shape, and I'm looking forward to merging it soon! Just a couple of minor tweaks and suggestions to further improve it and it's good to go — should only take just another minute of your time or so.

Thanks for putting this together! 🚀

obi1kenobi · 2023-10-05T03:27:08Z

src/lints/pub_function_changed_abi.ron

+                        span_: span @optional {
+                            filename @output
+                            begin_line @output
+                        }


Probably best if we grab the span info of the current function (instead of the baseline) so we can point to the file+line in the current code where the user should look.

obi1kenobi · 2023-10-05T03:29:13Z

test_crates/pub_function_changed_abi/new/src/lib.rs

+
+pub extern "Rust" fn implicit_c_function_becomes_rust() -> () {}
+
+pub extern "Rust" fn rust_function_remains_rust() -> () {}


Suggestion for a couple more cases here:

an explicitly-Rust ABI function remains Rust ABI but implicitly so, by removing the extern "Rust"

the same thing in the opposite direction: adding explicit Rust ABI to an already implicitly-Rust ABI function

These cases are important guards against possible false-positives, which are very annoying for users and so are at least as important to protect against as the test cases for true-positives that make sure our functionality continues to work.

obi1kenobi · 2023-10-05T03:29:58Z

src/lints/pub_function_changed_abi.ron

+        "public": "public",
+        "zero": 0,
+    },
+    error_message: "A publicly-visible function changed it's ABI type.",


Suggested change

error_message: "A publicly-visible function changed it's ABI type.",

error_message: "A publicly-visible function changed its ABI.",

Just consistency with the text below.

obi1kenobi · 2023-10-05T03:31:44Z

src/lints/pub_function_changed_abi.ron

+        "zero": 0,
+    },
+    error_message: "A publicly-visible function changed it's ABI type.",
+    per_result_error_template: Some("function {{join \"::\" path}} changed ABI type from {{abi_name}} to {{new_abi_name}} in {{span_filename}}:{{span_begin_line}}"),


Suggested change

per_result_error_template: Some("function {{join \"::\" path}} changed ABI type from {{abi_name}} to {{new_abi_name}} in {{span_filename}}:{{span_begin_line}}"),

per_result_error_template: Some("{{join \"::\" path}} changed ABI from {{abi_name}} to {{new_abi_name}} in {{span_filename}}:{{span_begin_line}}"),

We really want to try to fit this on one line in the terminal, so brevity is everything.

obi1kenobi · 2023-10-05T03:36:25Z

src/lints/pub_function_changed_abi.ron

+                        abi @fold @transform(op: "count")  @filter(op: "=", value: ["$zero"]) {
+                            name @filter(op: "=", value: ["%name"])
+                        }
+
+                        new_abi_: abi {
+                            name @output
+                        }


Functions are guaranteed to have exactly one ABI: we see this in the Trustfall schema where the ABI edge is defined abi: FunctionAbi! meaning exactly one neighboring vertex.

That makes this combination of query clauses equivalent to:

Suggested change

abi @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]) {

name @filter(op: "=", value: ["%name"])

}

new_abi_: abi {

name @output

}

new_abi_: abi {

name @filter(op: "!=", value: ["%name"]) @output

}

In the original, we say a combination of "there is no matching ABI" + "get the actual ABI here." In the revised version I'm suggesting, we just say "the ABI that exists is not a match." This is the same thing when there's exactly one ABI, no more and no less. If there could be zero or more than one ABI, that wouldn't be equivalent and we'd need a structure like the original one here.

src/lints/pub_function_changed_abi.ron

samuelrayment · 2023-10-05T20:26:27Z

Actually I'm not sure about the convention for the lint name.

I went with: pub_function_changed_abi but I've noticed the existing function lints omit the pub_. pub_static_missing does exist though, is there a convention for the pub_ prefix?

obi1kenobi · 2023-10-05T20:29:17Z

Actually I'm not sure about the convention for the lint name.

I went with: pub_function_changed_abi but I've noticed the existing function lints omit the pub_. pub_static_missing does exist though, is there a convention for the pub_ prefix?

Oof, we don't have any official convention but we probably should. Let's omit pub to stay consistent with the function lints. pub_static_missing should probably become static_missing as well — PR welcome and much appreciated if you'd like to put one together.

obi1kenobi · 2023-10-07T04:04:42Z

Heads up that I'm planning to publish a new release soon, probably on Sunday. I'd be happy to merge and ship this new lint, but it's also totally okay if we don't get it there in time. I just wanted to let you know my plans so you don't feel surprised to see it 👍 Thanks again for contributing!

samuelrayment · 2023-10-08T12:14:09Z

pub_ prefix now removed from the filenames and lint name.

obi1kenobi · 2023-10-08T15:25:55Z

Awesome stuff, merging! Thanks for putting it together, much appreciated 🚀

samuelrayment added 3 commits October 3, 2023 19:16

Adds lint for public function ABI change

43d45ae

Adds correct reference link for extern ABI

0bf69b2

Adds another lint example using extern "system"

9f44a08

samuelrayment force-pushed the pub_function_changed_abi branch from 710cb32 to 9f44a08 Compare October 3, 2023 18:16

samuelrayment added 2 commits October 3, 2023 20:15

Adds private extern function that should not be caught by the lint

40b94d2

Reformats pub function changed ABI query

aa5028f

obi1kenobi reviewed Oct 5, 2023

View reviewed changes

samuelrayment added 2 commits October 5, 2023 21:03

Adds more false positive test cases

0f95571

Simplifies query and shortens error messages

da99c0d

Removes pub_ prefix in lint name

b84724f

samuelrayment force-pushed the pub_function_changed_abi branch from 4453efc to b84724f Compare October 8, 2023 12:13

obi1kenobi approved these changes Oct 8, 2023

View reviewed changes

obi1kenobi merged commit c30bf3a into obi1kenobi:main Oct 8, 2023
32 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds lint for public function ABI change #546

Adds lint for public function ABI change #546

samuelrayment commented Oct 3, 2023

obi1kenobi left a comment

obi1kenobi Oct 5, 2023

obi1kenobi Oct 5, 2023

obi1kenobi Oct 5, 2023

obi1kenobi Oct 5, 2023

obi1kenobi Oct 5, 2023

obi1kenobi Oct 5, 2023

samuelrayment commented Oct 5, 2023

obi1kenobi commented Oct 5, 2023

obi1kenobi commented Oct 7, 2023

samuelrayment commented Oct 8, 2023

obi1kenobi commented Oct 8, 2023


		pub extern "Rust" fn implicit_c_function_becomes_rust() -> () {}

		pub extern "Rust" fn rust_function_remains_rust() -> () {}

	error_message: "A publicly-visible function changed it's ABI type.",
	error_message: "A publicly-visible function changed its ABI.",

	per_result_error_template: Some("function {{join \"::\" path}} changed ABI type from {{abi_name}} to {{new_abi_name}} in {{span_filename}}:{{span_begin_line}}"),
	per_result_error_template: Some("{{join \"::\" path}} changed ABI from {{abi_name}} to {{new_abi_name}} in {{span_filename}}:{{span_begin_line}}"),

Adds lint for public function ABI change #546

Adds lint for public function ABI change #546

Conversation

samuelrayment commented Oct 3, 2023

obi1kenobi left a comment

Choose a reason for hiding this comment

obi1kenobi Oct 5, 2023

Choose a reason for hiding this comment

obi1kenobi Oct 5, 2023

Choose a reason for hiding this comment

obi1kenobi Oct 5, 2023

Choose a reason for hiding this comment

obi1kenobi Oct 5, 2023

Choose a reason for hiding this comment

obi1kenobi Oct 5, 2023

Choose a reason for hiding this comment

obi1kenobi Oct 5, 2023

Choose a reason for hiding this comment

samuelrayment commented Oct 5, 2023

obi1kenobi commented Oct 5, 2023

obi1kenobi commented Oct 7, 2023

samuelrayment commented Oct 8, 2023

obi1kenobi commented Oct 8, 2023