non_topologically_sorted_functions linter

[J4CKVVH173]

Wrote a new linter with rules from this issue .

[CLAassistant]

All committers have signed the CLA.

[smoelius]

@J4CKVVH173 Sorry for the delay and thank you very much for working on this.

I'm going to need to give this at least one more pass, just FYI.

[smoelius]

There's a way to get a LocalDefId's body from a TyCtxt. I think it is this: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/struct.TyCtxt.html#method.hir_body_owned_by

Here are examples where that function is used in the Clippy repo: https://github.com/search?q=repo%3Arust-lang%2Frust-clippy%20hir_body_owned_by&type=code

[J4CKVVH173]

replaced this

[smoelius]

I think you should be able to use this to get a LocalDefId's span: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/struct.TyCtxt.html#method.def_span

Here are examples where that function is used in the Clippy repo: https://github.com/search?q=repo%3Arust-lang%2Frust-clippy+def_span&type=code

[J4CKVVH173]

replaced this with cx.tcx.def_span

[J4CKVVH173]

I was forced to roll back these changes. they break the visual representation of the error (you can see it in the tests in the previous comment), and more importantly, they break the entire check. I've reduced the use of spans, but I can't completely give it up.

[smoelius]

you can see it in the tests in the previous comment

Can you point me to what you are referring to, e.g., provide a link to a line in a log file?

[J4CKVVH173]

commit with this changes
bf5eb85

[smoelius]

I'm sorry, but I am still not following. What do you mean when you say the following?

they break the visual representation of the error (you can see it in the tests in the previous comment), and more importantly, they break the entire check

"you can see it in the tests in the previous comment" suggests to me that something in a log file should make this point clear. Am I interpreting that right?

If so, could you point me to the place in the log file that should make that point clear?

[J4CKVVH173]

You suggested to use def_span instead of let mut spans: HashMap<LocalDefId, Span> = HashMap::new();.

I replaced spans with def_span - you can see this in this commit bf5eb85. According to the types, everything fits, everything starts, but there is a nuance.

After these changes, this linter started to find errors where they originally did not exist.
Examples:

• https://github.com/trailofbits/dylint/actions/runs/19594579230/job/56117523563#step:11:1121
• https://github.com/trailofbits/dylint/actions/runs/19594579230/job/56117523563#step:11:1110

that's why I rolled back these changes and left working with spans.

[smoelius]

It's good that you noticed the false positives. I would not give up on the span approach, though.

I checked out bf5eb85, ran it, and observed the same false positives you did.

I made the following change to verify that both spans come from the same source file:

             .filter_map(|&(a, b)| {
                 let span_a = cx.tcx.def_span(a);
                 let span_b = cx.tcx.def_span(b);
-                if span_a.lo() > span_b.hi() {
+                let source_map: &SourceMap = &cx.sess().source_map();
+                if std::sync::Arc::ptr_eq(
+                    &source_map.lookup_source_file(span_a.lo()),
+                    &source_map.lookup_source_file(span_b.hi()),
+                ) && span_a.lo() > span_b.hi()
+                {
                     let name_a = cx.tcx.def_path_str(a.to_def_id());
                     let name_b = cx.tcx.def_path_str(b.to_def_id());
                     let violation = Violation {

After that, I still saw this warning, though:

error: 
   --> internal/src/clippy_utils/revs_no_preinstall.rs:185:5
    |
185 |     fn versions() {
    |     ^^^^^^^^^^^^^ function `clippy_utils::revs_no_preinstall::test::versions` should be defined before `clippy_utils::revs_no_preinstall::Revs::new`
    |
    = help: move the function earlier in the module so callers and callee ordering is respected
    = note: `-D non-topologically-sorted-functions` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(non_topologically_sorted_functions)]`

Those could probably be alleviated by recording the span of the module being checked, and verifying that spans being warned about in the module's span.

[J4CKVVH173]

I appreciated your suggestion, but I still don't understand what can be done here :)
I've been poking around, but I can't figure out how to do without the initial filtering. spans may not be a good name and is confusing, but its essence is that it preserves only the internal functions of the module. it seems to me that this "optimization" only leads to big problems.

[smoelius]

OK. I think you've convinced me. Please ignore this request.

I will give this PR another look within the next few days.

[smoelius]

How difficult would it be to add a note showing where the caller calls the callee?

[J4CKVVH173]

I made several attempts, but I couldn't display it :(

so unless it's very critical, I would suggest skipping it.

[smoelius]

Could you give me an idea of the types of errors you were getting?

If the lint's warning could show where function X calls function Y, it would provide some assurance that the lint was working correctly.

[J4CKVVH173]

I don't have any errors, I just can't find the way to display the warning with two place in the code at the same time with this formating like this

warning: 
  --> $DIR/multiple_incorrect.rs:15:1
   |
LL | fn bar() {}
   | ^^^^^^^^^^^ function `bar` should be defined before `baz`
   |
   = help: move the function earlier in the module so callers and callee ordering is respected
   = note: `#[warn(non_topologically_sorted_functions)]` on by default

warning: 1 warning emitted

I find the to point to one place at the warning

[J4CKVVH173]

If the lint's warning could show where function X calls function Y, it would provide some assurance that the lint was working correctly.

it is better to check this through automated tests.

[J4CKVVH173]

this is the bes attempt that i have achived

before:

warning: 
  --> $DIR/wrong_inner_call.rs:7:1
   |
LL | / fn foo() {
LL | |     bar();
LL | |     {
LL | |         baz();
LL | |     }
LL | | }
   | |_^ function `foo` should be defined before `baz`
   |
   = help: move the function earlier in the module so callers and callee ordering is respected
   = note: `#[warn(non_topologically_sorted_functions)]` on by default

warning: 
  --> $DIR/wrong_inner_call.rs:14:1
   |
LL | fn bar() {}
   | ^^^^^^^^^^^ function `bar` should be defined before `baz`
   |
   = help: move the function earlier in the module so callers and callee ordering is respected

warning: 2 warnings emitted

after:

warning: 
  --> $DIR/wrong_inner_call.rs:7:1
   |
LL | / fn foo() {
LL | |     bar();
LL | |     {
LL | |         baz();
   | |         ----- call site #1: `baz` called here
LL | |     }
LL | | }
   | |_^ function `foo` should be defined before `baz`
   |
   = help: move the function earlier in the module so callers and callee ordering is respected
   = note: `#[warn(non_topologically_sorted_functions)]` on by default

warning: 
  --> $DIR/wrong_inner_call.rs:14:1
   |
LL | fn bar() {}
   | ^^^^^^^^^^^ function `bar` should be defined before `baz`
   |
   = help: move the function earlier in the module so callers and callee ordering is respected
   = note: no call site recorded (internal)

warning: 2 warnings emitted

I'm not sure if this is what you need.

[smoelius]

A note is how I would do it, similar to how you did in the first of these two warnings.

That is, I would try to make the warning look like this (hand-drawn ascii art):

warning: 
  --> $DIR/wrong_inner_call.rs:7:1
   |
LL  fn foo() {
   | ^ function `foo` should be defined before `baz`
   |
   = help: move the function earlier in the module so callers and callee ordering is respected
   = note: `baz` is called from `foo` here
  --> $DIR/wrong_inner_call.rs:7:1
   |
LL |     baz();
   |
   = note: `#[warn(non_topologically_sorted_functions)]` on by default

If the order of the help and note are swapped, I think that is fine.

For the second of your two warnings ("no call site recorded (internal)"), I would just omit the note.

[J4CKVVH173]

made this conclusion