Inconsistent results between runs (again)

[dalemyers]

I opened a very similar ticket months ago: https://github.com/peripheryapp/periphery/issues/245 and it had a clear fix. Unfortunately, I'm (still?) seeing inconsistent results between runs (again?). This time, I can run it once, get a list of issues, run it again on exactly the same code (both are clean builds) and get different results.

It's not that one run is a subset of the other either. They have different results. Out of ~3900 issues (big project :) ) around ~3850 of them are the same. It's the final ones that are different. I can't see a pattern to them at all.

For example, there's a struct where most of its properties are marked as unused, but one run finds more than others.

The type of declaration varies as well. Properties, functions, classes, etc.

Presumably there has to be something non-deterministic in here?

[ileitch]

Are any of the results false positives? Or is it just sometimes omitting some correct results?

For example, there's a struct where most of its properties are marked as unused, but one run finds more than others.

Even though you said it affects many different kinds of declarations, let's try hone in one just case.

• Does this struct always appear in the results?
• Are any of its properties always identified as unused, or is it totally random?
• What protocols does it conform to?
• Does it have custom initializers?

[dalemyers]

It's an hour per run, so I'll script something on Monday to run some serious tests and see what we get.

[ileitch]

Are the results inconsistent also when using --skip-build? Or does skip-build take 1 hour? 😮

[dalemyers]

We never skip the build. It's always clean. And the full build and scan takes an hour. :)

[ileitch]

Ah, is this running in CI?

[dalemyers]

That's the eventual goal. We have 2 "phases":

1. We ran an initial scan, which generated those 3900 results. We added these as comments directly to the source code.
2. On every PR, we run a scan, remove the results which have already been commented on in the source, then add comments to the PR.

However, I'm not doing anything like this for the tests. Just running manually, copying the results to a text file, running again, copying to a different text file and then doing a diff.

[dalemyers]

So I've run the same scan 5 times and looked at the diffs (10 of them in total) and there is a pattern, but it's not clear. No two diffs are the same. Here's a sample of all 10 on screen at the same time:

You don't need to make out the details, but you can see that 6 of them all have the same block missing from the right hand side as the first difference. For 3 out of the remaining 4 diffs it's a different change, and then the final one has yet another change for the first one. For the big block that is missing, runs 1, 2 and 3 have that block, 4 and 5 don't. However, even though 1, 2 and 3 all have that block, they have their own other differences between them.

I've checked quite a lot of these samples and while most seem correct, I believe I've found at least one which is not correct. 4 of the 5 runs mark this property as being assigned but never used (just a static member in a struct so nothing fancy with protocols, etc. going on) when I can see it clearly being read as a key for a dictionary later on. The plot thickens....

[ileitch]

Unfortunately that's a known bug in Swift - https://bugs.swift.org/browse/SR-14162. I plan to get this fixed for Swift 5.6.

That still shouldn't cause inconsistent behavior though.

[dalemyers]

A consistent bug is easily dealt with :) What I don't understand is why it isn't consistent.

I assume that at a high level Periphery basically follows these steps:

1. Build the app
2. Index the code base (can be multi-threaded)
3. Build a call tree (or similar)
4. Analyze that tree for unused declarations

Step 2 can clearly be multithreaded easily. 3 and 4 can as far as I can tell, but would need to be done safely. Should we experiment with turning of multi-threading to see how that changes things?

Also, is there any other information I can provide? Even if I could provide the project, a million lines is probably too much to debug and I don't even know where to start for a minimal repro.

[ileitch]

2 & 3 are pretty much the same step, and yes that part is concurrent. Step 4 is where things can get a bit complicated. Before we actually walk the graph to identify the unused declarations, there's a whole host of mutations that need to happen; one of those is to identify assign-only properties. Curiously, this is where the problem was with your previous report #245 (though it was another user that provided a repo case, so perhaps you're still experiencing a different, but same issue).

As much as possible, could you please show me how the static property is declared (does it have any get/set/willSet/didSet?). Also, show me the function that reads the property. I'm curious to see if this method has multiple reads to this property.

[ileitch]

I suspect this most likely isn't a concurrency issue. Step 4 is single threaded, because it takes only a fraction of overall time compared to 1, 2 & 3. It's more likely due to the liberal use of sets. Sometimes we need to poke around in these sets to find things, but obviously they don't have a guaranteed order (I'd actually prefer it if they had a guaranteed different order on each program run, it'd help identify bugs like this during development).

[dalemyers]

It's literally just declared like:

private enum Whatever {
  static let someProperty = "some_value"
}

And then used like:

func doThing(someProperty: String?) {
  var myDict: [String: String] = [:]

  if let someProperty = someProperty {
    myDict[Whatever.someProperty] = someProperty
  }

  doOtherThing(myDict)
}

Nothing special in here at all.

[ileitch]

Hey @dalemyers, just an FYI that I have an idea that I want to investigate, I just haven't had any time recently.

[dalemyers]

No worries, I've also been busy which is why I haven't looked any deeper either.

[ileitch]

Hey @dalemyers, I think I've fixed the cause of the inconsistency. The issue arises when there are properties with identical names at different scopes. Because of the non-deterministic ordering of Set, sometimes the assign-only property analyzer would remove a reference for the incorrect property.

public class FixtureClass131 {
    static var someProperty: String?
    var someProperty: String

    public init() {
        someProperty = ""
        Self.someProperty = ""
    }

    public func someMethod() {
        _ = Self.someProperty
    }
}

In this example, the instance method someProperty would be inconsistently identified as assign-only.

[dalemyers]

Amazing! I have no idea how you managed to figure this out, but super glad that you did!

I've still got a ton on, so it will be a while before I can run this again to verify.

[dalemyers]

Ok finally back and looking at this again. The good news is that this change has made a significant improvement. The vast majority of differences are now gone. Unfortunately, that's not all of them. I'm still seeing a couple of differences in our codebase (just two consecutive runs, one after the other). Literally just 2 location where it was inconsistent across these runs. The results it finds are valid, just missing from the other run (and it was 1 found in one run, 1 found in another).

An interesting thing to note is that of the two locations where it found unused parameters, one of them it did find 3 unused ones. To me, that implies that the scanning is working fine, it's something that is happening when combining results. If it was generic race condition, I'd expect to see one of them go missing, not all 3 parameters to that method that are unused. Does that make any sense?

Running a third time produced the same output as the first one, so things are much, much closer now.

[dalemyers]

In some good news, I've managed to repro with the Wikipedia app: https://github.com/wikimedia/wikipedia-ios

The output has been stripped down to just the results and sorted.

run1.txt run2.txt

 $ diff run1.txt run2.txt
31d30
< /Users/dalemy/Projects/wikipedia-ios/Wikipedia/Code/Article as a Living Document/ArticleAsLivingDocViewController.swift:10:10: warning: Function 'showEditHistory()' is unused
71a71,72
> /Users/dalemy/Projects/wikipedia-ios/Wikipedia/Code/ArticleViewController.swift:73:14: warning: Property 'referenceWebViewBackgroundTapGestureRecognizer' is unused
> /Users/dalemy/Projects/wikipedia-ios/Wikipedia/Code/ArticleViewController.swift:843:9: warning: Property 'scrollViewAnimationCompletions' is unused

[dalemyers]

@ileitch Just pinging here to get this re-opened. :)

[ileitch]

Sorry @dalemyers, I totally forgot about this issue. Have you tried recently with Swift 5.7?

[ghost]

I have not. I've actually changed companies and tech stacks, so I'm not running Periphery any longer (or even working on a mac!). I guess Wikipedia can be your test case if you want to investigate further?

Thanks for all the help!

[ileitch]

Thanks, Dale. Best of luck with the new tech direction!