fix(rome_fs): Fix incorrect detection of infinite symlinks

[arendjr]

Summary

This makes a few changes to how potentially infinite symlinks are detected in order to prevent false positives:

• It maintains the logic that if traversal discovers a file that has already been discovered, it will be skipped, but no more diagnostics will be reported in this situations. This handles the situation where symlinks between directories cause an infinite cycle, since the directories themselves will be inserted into the interner. Given that this situation causes no other issues for Rome, I think it's not a problem that these kinds of infinite cycles are not being reported by Rome anymore.
• It explicitly checks whether symlinks point at other symlinks and recursively follows them if so. While doing this, it tracks the depth and bails with a DeeplyNestedSymlinkExpansion diagnostic if it exceeds a maximum depth (currently set to 3).
• Given the changed nature of the detection, DeeplyNestedSymlinkExpansion replaces InfiniteSymlinkExpansion and the diagnostic messages have been updated too.

Fixes #4193, #4395.

Test Plan

Tests are included in the PR. I also renamed some of the test folders used by tests, since running cargo test symlink would cause regular failures with parallel tests interfering with each other's directories.

There is still one issue with the tests in this PR: There's a new test case that tests two symlinks pointing at one another, but because the traversal happens in parallel the order of the diagnostics is undefined. This causes the test to be unstable currently, so I need a way to restrict this file to running on a single thread. I'm not sure how to do this with rayon, so suggestions would be welcomed. Update: Fixed now.

[netlify]

✅ Deploy Preview for docs-rometools canceled.

Built without sensitive environment variables

Name	Link
🔨 Latest commit	712cdaf
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/64c3c8759950910008df93b0

[ematipico]

What's the main reason we should follow the symbolic links for three levels? I think this is really important to understand and explain because we will need to document this behaviour.

[arendjr]

What's the main reason we should follow the symbolic links for three levels? I think this is really important to understand and explain because we will need to document this behaviour.

Frankly, it's rather arbitrary. If the value is 1, we wouldn't support symlinks to symlinks at all. While having symlinks to symlinks is probably not common, not supporting them at all is probably not what we want either, so I picked something higher. 2 would probably be sufficient for pretty much anyone, but just in case I picked 3 :)

To be honest I'm not sure I would even try documenting this. I would judge chances that anyone runs into this because they have even deeper nesting to be negligibly small. And if they do, the message saying they have too deeply nested should be clear enough. In other words, for all practical intends and purposes, I would expect this to be not really a user-facing issue unless someone has a really special setup.

[ematipico]

There is still one issue with the tests in this PR: There's a new test case that tests two symlinks pointing at one another, but because the traversal happens in parallel the order of the diagnostics is undefined. This causes the test to be unstable currently, so I need a way to restrict this file to running on a single thread. I'm not sure how to do this with rayon, so suggestions would be welcomed.

We have these cases in our test suite. We usually remove all the diagnostics messages from the console: https://github.com/rome/tools/blob/main/crates/rome_cli/tests/commands/check.rs#L1213-L1231

You could use the same trick

[arendjr]

We have these cases in our test suite. We usually remove all the diagnostics messages from the console: https://github.com/rome/tools/blob/main/crates/rome_cli/tests/commands/check.rs#L1213-L1231

Thanks! I've used a similar trick to just check for the highlights in the output that the test would expect, instead of using a snapshot here.

The CHANGELOG entry is added too.