Help with `Defs.split_defs_around` impl · contributing

I've been working on implementing the chaining design proposal and have been having a lot of trouble with an implementation for split_defs_around. I've tried a bunch of different things, but I've been unable to get it right. The combination of Vecs, indexes, phantom data, and memory has been tricky.

The implementation below works... the problem is that there are unecessary defs that remain because I'm just cloning the original and then replace the tags to keep for before and after.

The current implementation behaves correctly (as in programs compile and run fine), but there is a lot of duplication which makes debugging the AST when reading it manually, and it would also grow quite large on real programs.

// crates/compiler/parse/src/ast.rs

#[derive(Debug, Clone, PartialEq, Default)]
pub struct Defs<'a> {
    pub tags: std::vec::Vec<EitherIndex<TypeDef<'a>, ValueDef<'a>>>,
    pub regions: std::vec::Vec<Region>,
    pub space_before: std::vec::Vec<Slice<CommentOrNewline<'a>>>,
    pub space_after: std::vec::Vec<Slice<CommentOrNewline<'a>>>,
    pub spaces: std::vec::Vec<CommentOrNewline<'a>>,
    pub type_defs: std::vec::Vec<TypeDef<'a>>,
    pub value_defs: std::vec::Vec<ValueDef<'a>>,
}

#[derive(Debug, Clone, PartialEq)]
pub struct SplitDefsAround<'a> {
    pub before: Defs<'a>,
    pub after: Defs<'a>,
}

impl<'a> Defs<'a> {

    // For desugaring suffixed Defs we need to split around the current value
    pub fn split_defs_around(&self, target: usize) -> SplitDefsAround {
        let mut before = self.clone();
        let mut after = self.clone();

        before.tags = self.tags[0..target].to_vec();

        if target >= self.tags.len() {
            after.tags = self.tags.clone();
            after.tags.clear();
        } else {
            after.tags = self.tags[(target + 1)..].to_vec();
        }

        SplitDefsAround { before, after }
    }
}

Brendan Hansknecht (Apr 07 2024 at 17:12):

Luke Boswell (Apr 07 2024 at 22:36):

No, that is the problem with my current solution. The tags are indexes into the other Vecs. So I'm keeping type_defs and value_defs that dont have a tag pointing to them.

Brendan Hansknecht (Apr 07 2024 at 23:36):

Luke Boswell (Apr 08 2024 at 00:33):

If I use Vec::remove on the value_defs it shifts the indexes. Which means the tags now point to the wrong place, and I haven't found a way to update them all correctly

Brendan Hansknecht (Apr 08 2024 at 01:00):

Honestly, the best way would be to reference the original defs and only update the tags. But that might require larger complier changes

Brendan Hansknecht (Apr 08 2024 at 01:35):

Another option. Update Defs to instead of containing std::vec::Vec make it have a &'a bumpalo::Vec or something like that.

Brendan Hansknecht (Apr 08 2024 at 01:35):

Then cause it is a refernce, no need to copy it. just keep referencing the original data.

Brendan Hansknecht (Apr 08 2024 at 01:38):

But yeah, you may be stuck properly splitting the defs. If so, may be worth just walking the defs, copying them over and assigning to new indices.

Brendan Hansknecht (Apr 08 2024 at 01:38):

Luke Boswell (Apr 09 2024 at 08:03):

There are two functions which I have implemented which are almost identical and where I think the issue is, both are in crates/compiler/parse/src/ast.rs, replace_with_value_def and split_defs_around.

Unfortunately I've managed to break things somehow. I know it's an issue with the way I am handling spaces.

Luke Boswell (Apr 09 2024 at 08:05):

let space_before: &[CommentOrNewline<'a>] = {
    let start: usize = self.space_before[tag_index].start();
    let len: usize = self.space_before[tag_index].len();

    &self.spaces[start..(start + len)]
};