feat: New lint trait_no_longer_object_safe

[devanbenz]

• Adds a new lint for traits to see if they are still object safe

[devanbenz]

@obi1kenobi Having a few issues with this PR. I'm noticing that the test is failing with the following error:

thread 'query::tests_lints::trait_no_longer_object_safe' panicked at src/query.rs:342:13:
The query produced a non-empty output when it compared two crates with the same rustdoc.
Query trait_no_longer_object_safe produced incorrect output (./src/lints/trait_no_longer_object_safe.ron).

Differences (-expected (empty)|+actual (trait_now_doc_hidden/new)):
-{}
+{
+    "./test_crates/trait_now_doc_hidden/": [
+        {
+            "name": String("AliasedAsDocHidden"),
+            "path": List([
+                String("trait_now_doc_hidden"),
+                String("AliasedAsDocHidden"),
+            ]),
+            "span_begin_line": Uint64(44),
+            "span_filename": String("src/lib.rs"),
+            "visibility_limit": String("public"),
+        },
+        {
+            "name": String("PublicTraitHiddenVariant"),
+            "path": List([
+                String("trait_now_doc_hidden"),
+                String("PublicTraitHiddenVariant"),
+            ]),
+            "span_begin_line": Uint64(49),
+            "span_filename": String("src/lib.rs"),
+            "visibility_limit": String("public"),
+        },
+        {
+            "name": String("PublicTraitDocumentedWithStringHidden"),
+            "path": List([
+                String("trait_now_doc_hidden"),
+                String("PublicTraitDocumentedWithStringHidden"),
+            ]),
+            "span_begin_line": Uint64(62),
+            "span_filename": String("src/lib.rs"),
+            "visibility_limit": String("public"),
+        },
+    ],
+}

It looks like its picking up a query from a different lint. When I go through and try to debug what exactly is happening I see that https://github.com/devanbenz/cargo-semver-checks/blob/ea9a307765ea5d80b12bf8ada784019ad3f23cab/src/query.rs#L140 is outputting trait_no_longer_object_safe in the test crates returned array which looks okay. Looking here https://github.com/devanbenz/cargo-semver-checks/blob/ea9a307765ea5d80b12bf8ada784019ad3f23cab/src/query.rs#L325 getting the output of crate_pair_path I'm seeing

"./test_crates/function_changed_abi/"
"./test_crates/function_changed_abi/"
"./test_crates/trait_now_doc_hidden/"

Which is the incorrect crate path 🤔

[obi1kenobi]

Would you mind configuring your editor to always end files with a newline, then fixing up all the new files?

IIRC you use one of the IntelliJ editors, so something like Settings → Editor → General → Ensure line feed at file end on Save should do the trick.

[obi1kenobi]

Let's avoid the ?highlight=object bit, we can just reference the heading and let the user read without extraneous visual highlights.

Suggested change

	reference_link: Some("https://doc.rust-lang.org/stable/reference/items/traits.html?highlight=object#object-safety"),
	reference_link: Some("https://doc.rust-lang.org/stable/reference/items/traits.html#object-safety"),

[obi1kenobi]

"Object safe" is likely to be an unfamiliar concept to many Rustaceans, so this would be a great spot to explain it a bit more.

Suggested change

	description: "A trait is no longer object safe",
	description: "A trait is no longer object safe, meaning it can no longer be used as `dyn Trait`",

[devanbenz]

yep! I plan on going through and cleaning up the messages :)

[obi1kenobi]

This is the key line in that test output:

The query produced a non-empty output when it compared two crates with the same rustdoc.

This means that comparing a crate to itself (i.e. no changes whatsoever) triggered a lint. This is always a false-positive, since there was no source code change at all.

There's a minor logic error bug in your query. Make sure the object-safety property is different between the baseline and current :)

Two ideas for follow-up PRs, if you're open to them:

• Add a note to the CONTRIBUTING doc that the test cases run all lints on all test crates, so it's perfectly normal to see lint output from other crates if those crates have code that triggers your new lint. Also add a note describing the linting of crates with an identical copy of themselves, to catch false-positives.
• Update the test output in that test, to do a better job explaining why a lint reporting issues when comparing some rustdoc to itself is a problem. In retrospect, there's no reasonable way you could have understood what that test was trying to do based on that message, so it's a clear and unnecessary onboarding papercut.

[devanbenz]

Two ideas for follow-up PRs, if you're open to them:

• Add a note to the CONTRIBUTING doc that the test cases run all lints on all test crates, so it's perfectly normal to see lint output from other crates if those crates have code that triggers your new lint. Also add a note describing the linting of crates with an identical copy of themselves, to catch false-positives.
• Update the test output in that test, to do a better job explaining why a lint reporting issues when comparing some rustdoc to itself is a problem. In retrospect, there's no reasonable way you could have understood what that test was trying to do based on that message, so it's a clear and unnecessary onboarding papercut.

Amazing - was able to resolve. Going to take note of those pr ideas you indicated :D

[devanbenz]

Assuming that this is failing due to nightly v stable 🤔

[obi1kenobi]

Let's do this:

• Split the test crate and expected nightly test output out of this PR and into a separate PR. Having seen that it would pass right now, we'll leave that one as a draft and merge it in the future.
• That will leave the new lint unconditionally included and available on all Rust versions here in this PR. That we can merge today and ship to our users who run nightly or beta Rust right now.

I think this is the least complex solution that gets us most of the benefits without big drawbacks.

[obi1kenobi]

Will merge after applying a couple of minor nit fixes + after you've had a chance to open the draft PR with the test crate and test outputs.

[devanbenz]

@obi1kenobi #661 opened a draft PR with the tests