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

[zig] Parser: Add Region to node #7687

Merged

Conversation

gamebox
Copy link
Collaborator

@gamebox gamebox commented Mar 11, 2025

Adds the Region struct to all nodes. Needed for error reporting, and making formatting work correctly.

Copy link
Member

@bhansconnect bhansconnect left a comment

Choose a reason for hiding this comment

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

I think you should add regions to snapshot printing before merging this. That will add some test cases to verify the regions are matching up as expeted.

Comment on lines 715 to 745
node.tag = .decl;
node.data.lhs = d.pattern.id;
node.data.rhs = d.body.id;
node.region = d.region;
},
.expr => |expr| {
node.tag = .expr;
node.data.lhs = expr.expr.id;
node.region = expr.region;
},
.crash => |c| {
node.tag = .crash;
node.data.lhs = c.expr.id;
node.region = c.region;
},
.expect => |e| {
node.tag = .expect;
node.data.lhs = e.body.id;
node.region = e.region;
},
.@"return" => |r| {
node.tag = .@"return";
node.data.lhs = r.expr.id;
node.region = r.region;
},
.import => |i| {
node.tag = .import;
node.region = i.region;
node.main_token = i.module_name_tok;
var rhs = ImportRhs{
.aliased = 0,
Copy link
Member

Choose a reason for hiding this comment

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

Aren't all of these regions missing the body?

Like the region for return x should include the entire return x, no just be the region of the return token?
Though maybe I am missing something, I just don't see anything aggregating regions.

while (self.peek() != .CloseSquare and self.peek() != .EndOfFile) {
self.advance();
}
self.expect(.CloseSquare) catch {};
self.store.clearScratchExposedItemsFrom(scratch_top);
return self.pushMalformed(IR.NodeStore.HeaderIdx, .import_exposing_no_close);
return self.pushMalformed(IR.NodeStore.HeaderIdx, .import_exposing_no_close, start);
Copy link
Member

Choose a reason for hiding this comment

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

Can we name the malformed regions better? start suggest that a malformed node is always a single character and never has a true region.

@lukewilliamboswell
Copy link
Collaborator

I think you should add regions to snapshot printing before merging this. That will add some test cases to verify the regions are matching up as expeted.

@bhansconnect I agree with this... though I think we might want to do that in a follow up. I've experimented with it some and I think we have some issues with regions we need to shake out and it will be much easier if we can land this and my PR (with the snapshot improvements) and we have much better debug information.

@lukewilliamboswell lukewilliamboswell merged commit 2484c34 into roc-lang:main Mar 11, 2025
18 checks passed
@gamebox
Copy link
Collaborator Author

gamebox commented Mar 12, 2025

@bhansconnect I am learning about mismatches in regions right now doing multiline stuff

I actually meant to mark this as DO NOT MERGE, but that's my bad.

@gamebox gamebox deleted the formatter-handles-whitespace branch March 12, 2025 14:10
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.

3 participants