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

Haz3l binding uses #965

Draft
wants to merge 8 commits into
base: dev
Choose a base branch
from
Draft

Haz3l binding uses #965

wants to merge 8 commits into from

Conversation

agrsh
Copy link

@agrsh agrsh commented Jan 22, 2023

Intended behavior is to highlight all uses of a bound variable when the cursor is over a pattern. At the moment, there is code which prints the uses, but the UI portion hasn't been done yet.

@agrsh
Copy link
Author

agrsh commented Feb 6, 2023

Largely finished. Also incorporates some bug fixes to the co-ctx. At the moment, all uses of any binding in a pattern are highlighted orange -- it may be better to have different bindings highlighted different colors. The code itself can be polished a little (in particular, there are still some print statements littered around).

Copy link
Member

@disconcision disconcision left a comment

Choose a reason for hiding this comment

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

Looking pretty good... some small fixes. I think we might want to iterate a bit on the style though... the orange doesn't do a great job of visually associating the use sites with the pattern selection. I think we should maybe use the same color as for the pattern term indicator, but further differentiate the use decoration by maybe making it an outline instead of a solid box, or something like that. maybe a different stroke style. not sure how that will look but worth trying, or maybe some other variation I think.

@@ -155,6 +155,18 @@ let test_result_layer =
);
};

let get_usage_ids = (zipper, info_map) => {
/* bad style, does reason have let*? */
Copy link
Member

Choose a reason for hiding this comment

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

it does yes; search the codebase. you'll need to import OptUtil.Syntax

@@ -168,6 +180,16 @@ let deco =
~color_highlighting: option(ColorSteps.colorMap),
) => {
let unselected = Zipper.unselect_and_zip(zipper);
let use_ids = get_usage_ids(zipper, info_map);
let use_highlights =
List.flatten(
Copy link
Member

Choose a reason for hiding this comment

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

ListUtil.flat_map

let _ =
List.map(
usage => {
print_string(fst(usage));
Copy link
Member

Choose a reason for hiding this comment

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

print cleanup

@@ -319,6 +319,29 @@ module UPat = {
}
};
};

let rec get_all_bindings = (pat: t) => {
Copy link
Member

Choose a reason for hiding this comment

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

technically this can be obtained by just extracting the variables names from the pattern's context. but this is going to change a bit with ADTs, so it's fine to just leave it for now. maybe add a "TODO(andrew)" comment on it for me to remind me to update it to the new co-ctx system once its fully in place.

(item: Ctx.co_item) => {item.id},
List.flatten(VarMap.lookup_all(info_exp.free, var)),
);
print_string("co_ctx | ");
Copy link
Member

Choose a reason for hiding this comment

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

cleanup (multiple prints)

let updated_map: list(usage_info) =
List.map(
(usage_pair: usage_info) => {
let (var, ids) = usage_pair;
Copy link
Member

Choose a reason for hiding this comment

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

just destructure this immediately ie ((var, ids): usage_info) => {...

add(~self, ~free, union_m([m_scrut] @ pat_ms @ branch_ms));
};
}
and upat_to_info_map =
(
~ctx=Ctx.empty,
~mode: Typ.mode=Typ.Syn,
~body_ids: list(Id.t)=[Id.invalid],
Copy link
Member

Choose a reason for hiding this comment

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

these are the uses, correct? if so, i think they should just be called that, body_ids is vague. also why does it contain Id.invalid by default instead of being empty?

let self = Typ.Joined(Fun.id, branch_sources);
let free = Ctx.union([free_scrut] @ branch_frees);
/* let free = Ctx.union([free_scrut] @ branch_frees); */
Copy link
Member

Choose a reason for hiding this comment

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

cleanup

body.ids @ def.ids;
} else {
body.ids;
};
Copy link
Member

Choose a reason for hiding this comment

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

consider body_ids @ (is_rec ? def_ids : [])

let def_ctx = extend_let_def_ctx(ctx, pat, ctx_pat, def);
let (ty_pat, ctx_pat, _m_pat) =
upat_to_info_map(~mode=Syn, ~body_ids=body.ids @ def.ids, pat);
let (def_ctx, is_rec) = extend_let_def_ctx(ctx, pat, ctx_pat, def);
Copy link
Member

Choose a reason for hiding this comment

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

i feel like this approach with is_rec is a little indirect. not sure if it's the best way, but you could just do a comparison ctx == def_ctx and avoid having to alter extend_let_def

@cyrus- cyrus- marked this pull request as draft June 10, 2023 02:48
@cyrus- cyrus- added the needs-polish for PRs that are substantially complete but need final polish label Jun 10, 2023
@cyrus- cyrus- added the needs-merge for PRs that need a merge from dev label Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-merge for PRs that need a merge from dev needs-polish for PRs that are substantially complete but need final polish
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants