Break out semantics to library #1619
Conversation
|
I would like for introduce to live on the ast but there's some issues with cursor position for empty containers. To where if you get the ID from an expression in the AST and move to that in the zipper the cursor is outside of it. i.e on empty lists the cursor ends up outside the list. The only way I can see to address it is by having a path/zipper on the AST and a translation back to the tylr zipper. Sidenote: i think if we remove the is dependency for this the property tests will be a bit faster and debuggers will work. |
|
yeah i have the same issue with TyDi. there might be a slightly more lightweight option than full paths, e.g. term->term transformations plus an additional value representing an existing structural movement action to take after replacement, e.g. whether the caret should advance to the first hole after replacement. |
That's what I had done for introduce but Cyrus didn't like introduce on lists being singleton lists and otherwise there's no hole. Either way you have the same issue on empty strings. Maybe go to empty lists and string should go inside but that also seems sketchy. It's currently a flag to go left once |
|
I guess you could still abstract this; identify a more general notion of 'go to next point of interest', in the sense of per form defined ~ canonical cursor positions to effectuate structured transformations (eg middle of token for [|] to do [] => [?|]). (for tydi I went worse to tho, I just did all syntax construction as strings). |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## projector-gadts #1619 +/- ##
===================================================
+ Coverage 46.05% 46.18% +0.13%
===================================================
Files 126 126
Lines 14046 13991 -55
===================================================
- Hits 6469 6462 -7
+ Misses 7577 7529 -48
🚀 New features to boost your workflow:
|
|
Yeah, I'd have to think more about how it would look. Maybe there's only a single position for most forms. |
| | Typ => "type" | ||
| | Rul => "rule" | ||
| | Exp => "expression"; | ||
| include Semantics.Sort; |
There was a problem hiding this comment.
Are we planning on these diverging or is this just just an alias?
There was a problem hiding this comment.
just an alias, as Sort is used hundreds of times across tylr. not sure what best practices here should be; i definitely do this reluctantly. I only aliased Sort and Id in this manner
| statics: option(Semantics.Statics.Info.t), | ||
| /* Dynamic information about the syntax including | ||
| * live values of the syntax. Dynamics may be | ||
| * disabled by the user; this case (None) must be | ||
| * handled by projector authors */ | ||
| dynamics: option(Dynamics.Info.t), | ||
| dynamics: option(Semantics.Dynamics.Info.t), |
There was a problem hiding this comment.
You shouldn't need to namespace this given the open above.
| @@ -183,7 +183,8 @@ let gen_constructor_ident: QCheck.Gen.t(string) = | |||
| let* leading = char_range('A', 'Z'); | |||
| let+ tail = string_size(~gen=char_range('a', 'z'), int_range(1, 4)); | |||
| let ident = String.make(1, leading) ++ tail; | |||
| if (List.exists(a => a == ident, Haz3lcore.Form.base_typs)) { | |||
| //TODO(andrew): copied list of base types below to remove Form dep... | |||
| if (List.exists(a => a == ident, ["String", "Int", "Float", "Bool"])) { | |||
There was a problem hiding this comment.
I think we should reconsider this as well since (from what I recall) those are excluded as constructor names in parsing and I think that should happen in statics if anything. This doesn't happen for other non-base types.
There was a problem hiding this comment.
how would you handle this for the moment? remove this and leave it to statics to decide?
(seems reasonable to defer as much as possible to statics; the proximate situation is going to be that all of tylr, semantics, and the parser all implicitly depend on a shared but non-materialized language definition. i don't think we have an easy path for actually abstracting this is the implementation. this particular one (base types) already has multiple divergences in the implementation (i counted three different sets of base types used in different places))
There was a problem hiding this comment.
This needs to be consistent with the tylr version. Im fine with it being either hard coded here or a list defined in semantics somewhere. If we define it in semantics can't all 3 libraries use the same defn?
| settings.flip_animations && Action.should_animate(a) | ||
| ? Animation.request([Animation.Actions.move("caret")]) : (); |
There was a problem hiding this comment.
I know this is happening on perform now, but can I get some context.
There was a problem hiding this comment.
the animation system I created requires (a) capturing DOM state before an action is triggered, (b) storing that state so that it is persisted into the next MVU loop, and (c) capturing DOM state after the action has been formed, after the DOM is regenerated, but before draw occurs. (a) was done here; now i've moved it slightly so that Animation can live in Web. (b) happens in Main.re.
The reason why it was here to begin with (aside from the lazy fact that core has web dep due to projectors), is that in general we want to trigger animations based on the particular action taken, possibly with explicit dependence on both the before and after state of the model. I'm continually torn on whether effects like this should be handled as part of an entirely separate switch; i like the separation of concerns, but also think that it's not terrible to be forced to think about the effect when you change an action. I had started moving in the separation direction, but ended up punting, forgetting about it, and leaving it in an intermediate state. I think now that the effect should probably be abstracted to the Action.should_animate function, which should take the before and after models as well as the action, and produce an effect.
| @@ -87,7 +87,7 @@ type pos = | |||
| type spec = p(Zipper.t); | |||
There was a problem hiding this comment.
This file or at least locally below might benefit from opening Semantics
There was a problem hiding this comment.
My biggest question with the PR is the what the medium term plan for semantics is. Naming-wise it's a bit confusing semantics defines the AST of the language. I would expect it to just be typechecking, elaboration, dynamics. But that's a huge change.
There was a problem hiding this comment.
we could put term files in a separate target, but not clear to me what we gain from that at the moment. i look at this change as a first-order movement to obtain some basic directional separation between hazel-the-language and hazel-the-editor, motivated partially by thinking through clarifying projector versus tylr dependencies. alternatively, we could call this library language.
more broadly, i'm trying to iteratively clarify the overall structure of the codebase. it's not obvious to be the current web/core division makes sense, even in principle. increasingly i feel like we're going to want multiple components which can interact with the 'language server', and theses components may have tightly integrated backends and frontends, where the backend refers to some edit action calculi specific to how their UI is surfaced to the user. these components might reasonably decide to divide internally into front and back build targets, particularly if they want to support multiple frontends, but this feels different to be than enforcing a front-back distinction on the codebase entire. but the big incoming question i think is how grove fits in
There was a problem hiding this comment.
I think language ic clearer but I might be being pedantic.
we could put term files in a separate target, but not clear to me what we gain from that at the moment.
I don't think it's tenable/necessary know but long term I think the menhir parser, and tylr could only depend on the AST and not the semantics. It's also beneficial from an enforced separation of concerns standpoint.
it's not obvious to be the current web/core division makes sense, even in principle. increasingly i feel like we're going to want multiple components which can interact with the 'language server', and theses components may have tightly integrated backends and frontends, where the backend refers to some edit action calculi specific to how their UI is surfaced to the user. these components might reasonably decide to divide internally into front and back build targets, particularly if they want to support multiple frontends, but this feels different to be than enforcing a front-back distinction on the codebase entire. but the big incoming question i think is how grove fits in
I agree there's multiple concerns here. My main medium term priority is to have a build of tylr/semantics/syntax minus projectors so that we can run the non-web parts of hazel without the js of ocaml requirement. I don't think it requires fully separating the frontend/backend divide. For any components where we can inject their dependencies so that they can be tested or run on the backend.
There was a problem hiding this comment.
i went through and update dependencies somewhat; a lot of the tests depend purely on semantics now. probably breaking makterm and expToSeg out would do most of the rest?
| let consistent = (s, s') => | ||
| switch (s, s') { | ||
| | (Any, _) | ||
| | (_, Any) => true | ||
| | _ => s == s' | ||
| }; |
There was a problem hiding this comment.
Can we just add deriving eq to t and make consistent equal to equals?
| let to_string = | ||
| fun | ||
| | Any => "Any" | ||
| | Pat => "Pat" | ||
| | TPat => "TPat" | ||
| | Typ => "Typ" | ||
| | Rul => "Rul" | ||
| | Exp => "Exp"; |
There was a problem hiding this comment.
We can derive this using ppx_variants_conv.
| open Haz3lcore; | ||
| open Semantics; |
There was a problem hiding this comment.
Is there anything clashing if we open both?
| //TODO(andrew): ... | ||
| module Secondary2 = Secondary; | ||
| open Semantics; | ||
| module Secondary = Secondary2; |
There was a problem hiding this comment.
Is this a intermediate step while you're seeing if you can remove Semantics.Secondary? Otherwise I don't see why they can't be aliases.
This is a speculative breaking semantics out of core. Wanted to see how clean it was. Answer is: better than I thought, but not perfect.
Semantics contains what you'd expect, I think. Borderline cases are things like Operators which are maybe more aware of the surface syntax than is strictly seemly.
Haz3lcore is a bit more fraught. Currently it contains:
@Negabinary at this point I'm wondering if our first-order move here for splices is proceed with this PR, removing haz3lcore entirely and just moving all contents to haz3lweb, most but not all to a new tylr subfolder, but leaving the resolving the tylr/projectors API to future work.
TODO: