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

Add a file cache #186

Merged
merged 5 commits into from
Jan 28, 2024
Merged

Add a file cache #186

merged 5 commits into from
Jan 28, 2024

Conversation

ehllie
Copy link
Contributor

@ehllie ehllie commented Jan 26, 2024

I've added a file cache to ModuleCache. That should allow for passing in memory buffers from the lsp to the compiler, and therefore updating diagnostics without having to wait for a file write. It also changes error messages to be relative paths from the root each module is in, and as such the expected errors for tests had to be updated.

Additionally, I've separated the check function into a frontend module that is reused in the language server like previously suggested.

self.document_map.get(&uri).unwrap()
},
};

// TODO: This range seems to be off
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What exactly was off about it? Not sure if it's just fixed now because they seem fine to me:

image

Copy link
Owner

Choose a reason for hiding this comment

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

I have an example I'm working on currently where the location is off:
image

Running the compiler on the file reports:
image

Where the line and column info are correct.

Copy link
Owner

Choose a reason for hiding this comment

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

You can ignore the contents of the file. I've found a good way of making Ante thread-safe while also allowing aliased mutability without lifetime annotations!

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, I've determined this is because of larger unicode characters present in the file causing the internal byte index to be different than the character index.

jfecher
jfecher previously approved these changes Jan 28, 2024
Copy link
Owner

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

Really nice! Big improvements in the LS and in getting rid of the read file or panic function while displaying errors.

I think there are some changes we could make to reduce the cloning of files. Since I can't edit this PR myself though, I'll log the cases here, merge this, and make a follow up commit myself to reduce them to reduce your workload.

Thank you again! Do you have plans on what to work on for ante-ls next?

ante-ls/src/main.rs Show resolved Hide resolved
src/cache/mod.rs Outdated Show resolved Hide resolved
src/cache/mod.rs Show resolved Hide resolved
src/cache/mod.rs Show resolved Hide resolved
src/error/mod.rs Outdated Show resolved Hide resolved
src/frontend.rs Show resolved Hide resolved
@jfecher jfecher merged commit 59a9188 into jfecher:master Jan 28, 2024
1 check passed
@ehllie
Copy link
Contributor Author

ehllie commented Jan 28, 2024

Really nice! Big improvements in the LS and in getting rid of the read file or panic function while displaying errors.

I think there are some changes we could make to reduce the cloning of files. Since I can't edit this PR myself though, I'll log the cases here, merge this, and make a follow up commit myself to reduce them to reduce your workload.

Thank you again! Do you have plans on what to work on for ante-ls next?

Well, one way to reduce the cost of the clones would be using an Arc. I tired using an Rc at first, but it would not work inside async functions as Rc in not Send + Sync. I figured it's best not to prematurely optimise this, especially since at current scale it seems efficient enough.

I think I'm going to try implementing hover next.

let range = rope_range_to_lsp_range(loc.start.index..loc.end.index, &rope);
let message = format!("{}", diagnostic.display(&cache));
Copy link
Owner

Choose a reason for hiding this comment

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

Also, it looks like this change on master was accidentally reverted.

@jfecher
Copy link
Owner

jfecher commented Jan 28, 2024

Well, one way to reduce the cost of the clones would be using an Arc. I tired using an Rc at first, but it would not work inside async functions as Rc in not Send + Sync. I figured it's best not to prematurely optimise this, especially since at current scale it seems efficient enough.

Returning a Cow works, see 38fab45

@ehllie ehllie deleted the file-cache branch January 28, 2024 01:37
ehllie added a commit to ehllie/ante that referenced this pull request Feb 8, 2024
After jfecher#186 all module paths should be absolute, and as such it's not necessary to prefix them with ./
jfecher pushed a commit that referenced this pull request Feb 23, 2024
* Implement named constructor

* Fix llvm backend trying to use relative paths

After #186 all module paths should be absolute, and as such it's not necessary to prefix them with ./
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