Config tree

[cormacrelf]

Linkage: rust-lang#16254

Ok, another PR. This is what I made, and it was fun, but I think you should probably throw most of it out and use some salsa queries, because I ended up doing quite a lot of manual invalidation.

Basically, the tree is represented by a bunch of parent relationships in ConfigChanges.parent_changes. When you load your workspace, you std::path::Path::ancestors + .join("rust-analyzer.toml") your way to a hash map of FileId -> parent rust-analyzer.toml FileId. This implementation doesn't assume the FileIds will exist, so the Vfs::alloc_file_id method is made public.

This PR does not handle constructing the parent hashmap, but it does implement the tree itself and actually overriding the configs on demand.

The good bits possibly to keep:

• struct ConfigChanges + enum ConfigParent + ConcurrentConfigTree::apply_changes to set the salsa inputs if they've changed
• The parent data is stored via #[salsa::input] fn parent_ra_toml(FileId) -> FileId
• The client config is stored via another salsa::input
• ConcurrentConfigTree::local_config(FileId) -> Arc<LocalConfigData> becomes a salsa query. Ignore the RwLock stuff, salsa does that.
  • The top level query overrides with the client config. The thing it overrides is the output of compute_recursive.
  • compute_recursive also becomes a query. It should return some default value if you accidentally make it cyclic (i.e. parent_ra_toml(x) == x) lest we crash salsa
• You can probably keep the test at the bottom, only changing the constructor.

[cormacrelf]

Ok, may or may not have implemented what I described there. Now it uses salsa, and it's a lot simpler.

[cormacrelf]

I think it might even be possible to change the API such that you just pass in project root + source roots paths (not vfspaths) + a &Vfs, and it does the path.ancestors() thing for you.

[alibektas]

Hi @cormacrelf . You did a lot, thank you for all this. Would you mind informing me about what else you are planning to do, so that I can concentrate on the rest of the issue?

[cormacrelf]

Well, you don't have to use it, I know you wanted to target the case without parent dirs etc. I have absolutely no problem with waiting until after getting an MVP out. But it does work, and it would be quite an effort again to make an MVP without it, because it handles a lot of things.

This was very greenfield, and I don't have a great sense of how to tie it in. The API I exposed was a bit of a shot in the dark, so I'm not sure if it will be exactly right. So the next step is integrating it, which is tough work for me as I am not very familiar with the project loading and Vfs change ingestion.

As for what else I can do: I know it definitely needs a way to filter out irrelevant Vfs changes, and only see changes related to ra.toml files. (Done.) Beyond that, I would need some guidance. For example:

1. Should it expose the GlobalConfigData as well? We are already parsing it, and we know which one is the project root, but how that ties into the project loading lifecycle, I don't know. If you did expose it, it might work like this, which seems pretty good:
   1. set project root, set source roots to the empty set, and load the client config in the first ConfigChanges.
   2. Put the root ra.toml on the list of vfs paths to load up, wait for that, and apply the vfs file changes.
   3. get global config and use it to determine how to load the source roots;
   4. load the source roots and apply them in a second ConfigChanges.
2. Is there ever more than one project root? What even are "linked projects"? I haven't the faintest idea. It currently only supports one.
3. It does not link a SourceRoot to its ra.toml's FileId. The whole API is pure FileIds at the moment.
4. When/if we get all the AbsPaths of all the loaded source roots together all at once. If not, is there an alternative way we can shape the API (like fn add_source_root()) that will be easier to use with the existing project loading infrastructure?

Any of those things I'm happy to help out on, especially insofar as it requires modifying the code I've already written. If you want to go ahead with this, you should look at the tests to see how the API works, and generally figure out how we're going to call it, which will hopefully clarify the questions above.

[cormacrelf]

I'm going to un-draft this, as it's in pretty good shape and ready for integration work.

[alibektas]

I cannot be sure what the big guns ( I am still fairly new to the project ) maintaining this project would think about changing Vfs. But I also understand that you need this to add dummy ratoml nodes

[cormacrelf]

I saw in a few places in the codebase the pattern

vfs.set_file_contents(path.clone(), ...); // returns a bool that is not used once
let file_id = vfs.file_id(path);

And it seemed silly to be cloning paths to do that when the alloc_file_id API was right there. We can clearly do it this way as well, even supplying None as the contents, so we don't technically need this to get the maybe-dummy file ids, it's just a waste of allocations. If anyone kicks up a fuss about it ,it's got a rational use case, and solves an awkward API problem that has come up before.

[alibektas]

Overall I liked this approach. The things we can possibly talk about are

• the functions that mutably borrow Vfs
• how you imagine fetching the FileId of the corresponding ratoml for a source file would look like because we can either
  • fetch the path for a FileId, get the dirname of it and then append .rust-analyzer.toml and look for its FileId. The reason I don't like this is that we are supposed to use interned ids instead of relying on conversion to real-world information such as paths ( dev docs also mention this) .
  • have a huge map that maps FileId -> FileId. Extremely inefficient

[alibektas]

Why don't you have a single apply_changes instead of apply_changes + apply_changes_inner?

[cormacrelf]

I wrote apply_changes one on top of apply_changes_inner. This is just how the code evolved. The _inner one is completely free of any path information, works entirely on FileIds, which I think is nice, given the stress placed on being able to do that in the dev docs. I figured at some point we would make this code work for virtual paths as well as AbsPathBufs, and on that day, apply_changes_inner will remain unchanged.

[cormacrelf]

Notably, apply_changes_inner does not require &mut Vfs. So it helps isolate that unsavoury part in apply_changes as well, and you are welcome to split it off if you want to put the Vfs mutation closer to the other Vfs mutations that we already do.

[cormacrelf]

how you imagine fetching the FileId of the corresponding ratoml for a source file would look like because we can either

A mapping of SourceRootId to FileId is my best guess. Source roots are 1:1 with leaf nodes in the config tree. Each SourceRootId has a FIleSet which (I assume) is used to map FileIds back to their source roots. So you can get from FileId to the rust-analyzer.toml FileId in two quite efficient hops.

Edit: not the FileSet, but rather the SourceDatabase's file_source_root input is precisely the mapping of FileId to SourceRootId I was hoping to find.

Edit 2: you could probably add SourceRootIds to the ConfigChanges set_source_roots field by turning the HashSet into a HashMap<SourceRootId, AbsPathBuf>.

[alibektas]

how you imagine fetching the FileId of the corresponding ratoml for a source file would look like because we can either

A mapping of SourceRootId to FileId is my best guess. Source roots are 1:1 with leaf nodes in the config tree. Each SourceRootId has a FIleSet which (I assume) is used to map FileIds back to their source roots. So you can get from FileId to the rust-analyzer.toml FileId in two quite efficient hops.

Edit: not the FileSet, but rather the SourceDatabase's file_source_root input is precisely the mapping of FileId to SourceRootId I was hoping to find.

Ok now it finally makes sense to me 😊 thank you