Skip to content
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

(fix) case insenstive file system document sync #1697

Merged
merged 15 commits into from
Nov 15, 2022

Conversation

jasonlyu123
Copy link
Member

#1627 Adopted the casing conversion helper function from typescript. And added two collections for tracking files case intensively in case-insensitive file systems. Another goal is to also address the famous "files differ only in casing" typescript error when it's VSCode using the old casing before renaming.

The strategy is that we store the lowercase filenames in the collection. And when retrieving or checking the existence of files. We convert it to lowercase before checking. And we keep the original casing in another place so we can preserve the casing. Just not sure if we need to keep track of multiple. Not sure if we should reach the filesystem to check the correct casing if there're multiple exists. Because the only way I can think of is to use the read directory of the parent directory. Not sure if it's worth the effort.

Created as a draft to discuss if the overall strategy has any flaws. Also need to scan through the codebase to check for any filename equality check. We might also need to normalize the casing before checking.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

This makes sense to me! About "so we can preserve the casing" -> what for exactly? If the system is case insensitive by definition there can't be multiple files which only are different in casing.

packages/language-server/src/utils.ts Show resolved Hide resolved
@jasonlyu123
Copy link
Member Author

Keeping the original is so that the hover info of a module path would show the original casing. And also that the URI converted from the typescript response is the original casing. It probably only happens in cross-document features so it probably only in typescript features.

It's mostly the non-project files that would have the difference. Because the casing typescript uses is from the getScriptFileNames and I currently prioritize the casing from project files.

About the multiple, originally I was thinking it might be more compatible with the old code. But maybe that is just hiding potential bugs. We should convert the casing before comparing.

After writing these. I think I can remove the original casing from the FileMap and only keep the original casing in the DocumentSnapshot and change the getScriptFileNames to use the DocumentSnapshot.filePath. If we found any non-typescript features needing the filePath casing we can use Document.getFilePath() or Document.uri to track the original casing.


const workspaceEdit = await updateImportsProvider.updateImports({
// imported files both old and new have to actually exist, so we just use some other test files
oldUri: pathToUrl(join(testFilesDir, 'diagnostics', 'diagnostics.svelte')),
oldUri: pathToUrl(join(testFilesDir, 'imported.svelte')),
Copy link
Member Author

Choose a reason for hiding this comment

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

This comment seems to be outdated. It now seems to be able to check even if the new file does not exist.

@jasonlyu123 jasonlyu123 marked this pull request as ready for review November 11, 2022 05:08
@jasonlyu123
Copy link
Member Author

I have scanned through the codebase for the Map<string, T>, Set<string>, path and file keywords. It's ready for review.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Mostly looks good, left some comments. The thing I'm most concerned about (but a that regex is likely very fast) is the cost of converting the file names that often - i.e. if it has a noteable impact on performance.

) {
this.onSnapshotChange = this.onSnapshotChange.bind(this);
this.globalSnapshotsManager.onChange(this.onSnapshotChange);
this.documents = new FileMap(useCaseSensitiveFileNames);
this.projectFileToOriginalCasing = new FileMap(useCaseSensitiveFileNames);
Copy link
Member

Choose a reason for hiding this comment

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

Not quite understand the use case if this map here - it's not set with original,case_insensitive or similar, it's set with originalFileName, originalFileName in all places where it's called

Copy link
Member Author

Choose a reason for hiding this comment

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

It's essentially Map<case_insensitive, original>. I think I can change it to directly use Map<case_insensitive, original> so it's more clear on its use case.

Copy link
Member

Choose a reason for hiding this comment

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

What I meant is that everywhere where set is invoked you call it with the same variable for both parameters

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Because the FileMap will transform the casing for the key but not the value. That is indeed confusing as it keeps two different things.

...snapshotManager
.getFileNames()
.filter(
(file) => !canonicalProjectFileNames.includes(getCanonicalFileName(file))
Copy link
Member

Choose a reason for hiding this comment

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

I believe this method is called rather often, and this would make it O(n^2) - can we use a Set instead?

jasonlyu123 and others added 4 commits November 15, 2022 12:27
Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
to be more clear on the intention
Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Thank you so much for implementing this!

@dummdidumm dummdidumm merged commit 85b11c7 into sveltejs:master Nov 15, 2022
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.

None yet

2 participants