Skip to content

enforce overload declaration ordering when computing the highlighted differences #1250

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

QuietMisdreavus
Copy link
Contributor

Bug/issue #, if applicable: rdar://155975575

Summary

When performing difference highlighting in the experimental overloaded symbol presentation (added in #928), the "longest common subsequence" is found based on the declaration fragments of all the overloaded symbols. However, there are some situations in which the order of declarations affects the resulting sequence, specifically when there can be multiple solutions with the same length. This can lead to situations where the same content can produce different rendered output, depending on which symbol's highlighting is computed first (since the results are memoized as of #992).

This PR removes this nondeterminism by enforcing that the declarations are ordered by their display order before computing the LCS.

Dependencies

None

Testing

Reducing this problem into a unit test is difficult, because the problem may not surface in a very small framework without many repetitions. The symbol graph added for this PR's test was modified from its original output to ensure the test data matched the real-world symbols that were exhibiting the issue, and even then the issue actually failed to show up in local testing. Still, the test does effectively demonstrate the issue.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran the ./bin/test script and it succeeded
  • [ n/a ] Updated documentation if necessary

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@sofiaromorales
Copy link
Contributor

Thanks for working on this, @QuietMisdreavus! The added unit test passes when reverting the logic to its original state. Is this because of its nondeterministic nature?

Copy link
Contributor

@sofiaromorales sofiaromorales left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code change make sense to me! The only bit I feel it could use some more explanation is the test. Why does it pass when the changes are reverted?

@QuietMisdreavus
Copy link
Contributor Author

Correct; the test passes without the code change because the issue is nondeterministic. The best explanation i could muster is that the problem only really occurs when there are many symbols in the symbol graphs, causing there to be more opportunities for concurrentPerform to load symbols out-of-order. When a symbol is computing its overload differences, it orders them as [current symbol] + [remaining overloads], meaning that the initial state for the LCS algorithm starts from that symbol's declaration. Since the first symbol to be loaded wins, it all comes down to which symbol is loaded first.

...and now that i have typed all that out, i want to try rewriting the test to generate a symbol graph with many symbols in it to try to induce this behavior. 🤔 Let me try that now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants