`undocumented_unsafe_blocks` does not trigger on unsafe trait impls #8761

tamaroning · 2022-04-29T16:25:49Z

changelog: This lint checks unsafe impls NOT from macro expansions and checks ones in macro declarations.
~~unsafe impls from macro invocations don't trigger the lint for now.~~
~~This lint checks unsafe impls from/not from macro expansions~~

rust-highfive · 2022-04-29T16:25:52Z

r? @giraffate

(rust-highfive has picked a reviewer for you, use r? to override)

giraffate · 2022-05-02T00:28:41Z

Sorry, I can't take much time this week, but next week I can take time for a review.

@Jarcho Could you do a review on this?

Jarcho · 2022-05-02T16:15:59Z

clippy_lints/src/undocumented_unsafe_blocks.rs

+            item_and_spans.push((item, item.span));
+        }
+        // Sort items by start position
+        item_and_spans.sort_by_key(|e| e.1.lo());


Item's are already in declarations order, so there's no need to sort.

Jarcho · 2022-05-02T17:08:39Z

clippy_lints/src/undocumented_unsafe_blocks.rs

@@ -86,6 +87,65 @@ impl LateLintPass<'_> for UndocumentedUnsafeBlocks {
            );
        }
    }
+
+    fn check_mod(&mut self, cx: &LateContext<'_>, module: &'_ hir::Mod<'_>, mod_span: Span, hir_id: hir::HirId) {


This should be using check_item rather than check_mod. Items can appear within functions as well as modules. A combination of clippy_utils::get_parent_node and scanning for the id of the contained item can be used to get the span to use for the search bounds.

Jarcho · 2022-05-03T01:59:50Z

clippy_lints/src/undocumented_unsafe_blocks.rs

+                    } else {
+                        // unsafe impl S {} /* comment */ unsafe impl T {}
+                        //                 ^-------------^
+                        item_and_spans[idx - 1].1.between(*item_span)


This will break if the previous item is from a macro expansion. You'll need to use rustc_span::hygiene::walk_chain(item_and_spans[idx - 1].1, SyntaxContext::root())

Jarcho · 2022-05-03T02:02:16Z

clippy_lints/src/undocumented_unsafe_blocks.rs

+        for (idx, (item, item_span)) in item_and_spans.iter().enumerate() {
+            if let hir::ItemKind::Impl(imple) = &item.kind
+                && imple.unsafety == hir::Unsafety::Unsafe
+                && !item_span.from_expansion()


Optional extension here to handle macro expansion from the same crate. See block_has_safety_comment for how that's done for unsafe blocks.

A call to is_unsafe_from_proc_macro is needed. You can change the function to take a Span rather than a Block.

Jarcho · 2022-05-03T02:08:54Z

clippy_lints/src/undocumented_unsafe_blocks.rs

+
+                    if let Ok(start) = source_map.lookup_line(span_before_impl.lo())
+                        && let Ok(end) = source_map.lookup_line(span_before_impl.hi())
+                        && let Some(src) = start.sf.src.as_deref()


You'll need to add a check here that the file is the same for both line lookups. This is more a sanity check then anything. Two spans from the same SyntaxContext in the same module should be in the same file, but I can't say for certain that's the case.

Jarcho · 2022-05-03T03:00:20Z

clippy_lints/src/undocumented_unsafe_blocks.rs

+                && !is_lint_allowed(cx, UNDOCUMENTED_UNSAFE_BLOCKS, hir_id)
+            {
+                // Checks if the lines immediately preceding the impl contain a safety comment.
+                let impl_has_safety_comment = {


Minor nit here. This block isn't needed. The first statement can be moved out, then the if assigned directly.

bors · 2022-05-05T13:33:28Z

☔ The latest upstream changes (presumably #8788) made this pull request unmergeable. Please resolve the merge conflicts.

tamaroning · 2022-05-07T11:13:19Z

Thanks for your review @Jarcho
I fixed code.

Jarcho · 2022-05-09T13:31:04Z

clippy_lints/src/undocumented_unsafe_blocks.rs

+
+/// Checks if the lines immediately preceding the item contain a safety comment.
+fn item_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>) -> bool {
+    if span_from_macro_expansion_has_safety_comment(cx, item.span) || span_in_body_has_safety_comment(cx, item.span) {


This should be returning false if the item is within a function, but doesn't have a safety comment. e.g.

unsafe trait Foo {} fn f() { Struct S; // no comment unsafe impl Foo for S {} }

Not a particularly big deal on it's own, but it contributes to the ICE later.

There's still duplicate work here. Easiest fix would be to remove the call to span_in_body_has_safety_comment as it's handled afterwards when matching on Node::Stmt(_).

Jarcho · 2022-05-09T13:33:24Z

clippy_lints/src/undocumented_unsafe_blocks.rs

+            let mut hi = false;
+            if let Node::Item(parent_item) = parent_node {
+                if let ItemKind::Mod(parent_mod) = &parent_item.kind {
+                    for (idx, item_id) in parent_mod.item_ids.iter().enumerate() {


It would be better to use Iterator::find_map here. the extraction of either Span::lo or Span::hi could be done here as well.

This should get rid of the need to have span_before_item and hi as mutable.

Jarcho · 2022-05-09T13:34:03Z

clippy_lints/src/undocumented_unsafe_blocks.rs

+                    }
+                }
+            }
+            let span_before_item = span_before_item.unwrap();


This can fail when the impl is inside a function and I think the crate root.

Jarcho · 2022-05-09T13:43:37Z

clippy_lints/src/undocumented_unsafe_blocks.rs

+            let span_before_item = span_before_item.unwrap();
+
+            let source_map = cx.sess().source_map();
+            if let Some(item_span) = walk_span_to_context(item.span, SyntaxContext::root())


walk_span_to_context isn't needed here. item.span is already at the root context.

tamaroning · 2022-05-10T15:03:36Z

Thanks for your review @Jarcho
I fixed all and added some test cases which contain impls in a function and ones in the crate root.

Jarcho

Just two more small things. I think it's good otherwise, but I'll take one more look at it later. Thanks for keeping up with the review.

Jarcho · 2022-05-11T15:14:35Z

clippy_lints/src/undocumented_unsafe_blocks.rs

+
+/// Checks if the lines immediately preceding the item contain a safety comment.
+fn item_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>) -> bool {
+    if span_from_macro_expansion_has_safety_comment(cx, item.span) || span_in_body_has_safety_comment(cx, item.span) {


There's still duplicate work here. Easiest fix would be to remove the call to span_in_body_has_safety_comment as it's handled afterwards when matching on Node::Stmt(_).

Jarcho · 2022-05-11T15:16:02Z

clippy_lints/src/undocumented_unsafe_blocks.rs

+    if item.span.ctxt() == SyntaxContext::root() {
+        if let Some(parent_node) = get_parent_node(cx.tcx, item.hir_id()) {
+            let comment_start;
+            match parent_node {


Small nit. This could be let comment_start = match parent_node { The delayed initialization makes it seem like something complicated is coming up.

tamaroning · 2022-05-13T08:45:31Z

Thanks! I removed the call to span_in_body_has_safety_comment, but I am not sure impls in a body
always has a Node::Stmt(..) as a parent node and a Node::Block(..) as a grandparent node.

Jarcho · 2022-05-13T13:13:10Z

The only way an item can appear in a function is through StmtKind::Item, and the only way a statement can appear is through a block.

Jarcho · 2022-05-15T13:45:37Z

Everything looks good now. Good job.

@giraffate

tamaroning · 2022-05-15T13:55:20Z

Thank you so much! @Jarcho

giraffate · 2022-05-15T23:56:59Z

@bors r=Jarcho

@tamaroning @Jarcho It looks good, thanks!

bors · 2022-05-15T23:57:01Z

📌 Commit 9e66f2d has been approved by Jarcho

`undocumented_unsafe_blocks` does not trigger on unsafe trait impls Closes #8505 changelog: ~~`unsafe impl`s from macro invocations don't trigger the lint for now.~~ ~~This lint checks unsafe impls from/not from macro expansions~~ This lint checks unsafe impls NOT from macro expansions and checks ones in macro declarations.

bors · 2022-05-15T23:57:07Z

⌛ Testing commit 9e66f2d with merge df43ccb...

bors · 2022-05-15T23:57:38Z

💔 Test failed - checks-action_test

Jarcho · 2022-05-16T00:05:26Z

You'll need to add changelog: ... on a single line in the PR body (first post).

giraffate · 2022-05-16T00:47:06Z

@bors retry

bors · 2022-05-16T00:47:12Z

⌛ Testing commit 9e66f2d with merge a1632ff...

bors · 2022-05-16T01:02:16Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing a1632ff to master...

rust-highfive assigned giraffate Apr 29, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 29, 2022

Jarcho requested changes May 3, 2022

View reviewed changes

tamaroning added 2 commits May 7, 2022 00:05

Resolved conflicts

5d0ca74

fix

8d85889

tamaroning force-pushed the fix_8505 branch from e3da210 to 8d85889 Compare May 7, 2022 10:50

Jarcho requested changes May 9, 2022

View reviewed changes

fix

aaf87c3

chore

1926217

Jarcho requested changes May 11, 2022

View reviewed changes

fix

9e66f2d

giraffate approved these changes May 15, 2022

View reviewed changes

bors merged commit a1632ff into rust-lang:master May 16, 2022

BorisBoutillier mentioned this pull request Aug 13, 2022

[Merged by Bors] - Fix for bevy CI on main - clippy safety comments on trait. bevyengine/bevy#5665

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`undocumented_unsafe_blocks` does not trigger on unsafe trait impls #8761

`undocumented_unsafe_blocks` does not trigger on unsafe trait impls #8761

tamaroning commented Apr 29, 2022 •

edited by giraffate

Loading

rust-highfive commented Apr 29, 2022

giraffate commented May 2, 2022

Jarcho May 2, 2022

Jarcho May 2, 2022

Jarcho May 3, 2022

Jarcho May 3, 2022

Jarcho May 3, 2022

Jarcho May 3, 2022

bors commented May 5, 2022

tamaroning commented May 7, 2022

Jarcho May 9, 2022 •

edited

Loading

Jarcho May 11, 2022

Jarcho May 9, 2022

Jarcho May 9, 2022

Jarcho May 9, 2022

tamaroning commented May 10, 2022 •

edited

Loading

Jarcho left a comment

Jarcho May 11, 2022

Jarcho May 11, 2022 •

edited

Loading

tamaroning commented May 13, 2022

Jarcho commented May 13, 2022

Jarcho commented May 15, 2022

tamaroning commented May 15, 2022

giraffate commented May 15, 2022

bors commented May 15, 2022

bors commented May 15, 2022

bors commented May 15, 2022

Jarcho commented May 16, 2022

giraffate commented May 16, 2022

bors commented May 16, 2022

bors commented May 16, 2022

undocumented_unsafe_blocks does not trigger on unsafe trait impls #8761

undocumented_unsafe_blocks does not trigger on unsafe trait impls #8761

Conversation

tamaroning commented Apr 29, 2022 • edited by giraffate Loading

rust-highfive commented Apr 29, 2022

giraffate commented May 2, 2022

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

bors commented May 5, 2022

tamaroning commented May 7, 2022

Jarcho May 9, 2022 • edited Loading

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

tamaroning commented May 10, 2022 • edited Loading

Jarcho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jarcho May 11, 2022 • edited Loading

Choose a reason for hiding this comment

tamaroning commented May 13, 2022

Jarcho commented May 13, 2022

Jarcho commented May 15, 2022

tamaroning commented May 15, 2022

giraffate commented May 15, 2022

bors commented May 15, 2022

bors commented May 15, 2022

bors commented May 15, 2022

Jarcho commented May 16, 2022

giraffate commented May 16, 2022

bors commented May 16, 2022

bors commented May 16, 2022

`undocumented_unsafe_blocks` does not trigger on unsafe trait impls #8761

`undocumented_unsafe_blocks` does not trigger on unsafe trait impls #8761

tamaroning commented Apr 29, 2022 •

edited by giraffate

Loading

Jarcho May 9, 2022 •

edited

Loading

tamaroning commented May 10, 2022 •

edited

Loading

Jarcho May 11, 2022 •

edited

Loading