Add positions to events (issue #37) #69

wzwywx · 2023-10-21T08:09:17Z

This is a draft PR to improve the diagnostics message as indicated in issue 37.

I've managed to add the position of the return to the ReturnedTypeDoesNotMatch error.

If the solution is acceptable, I can do the same for the other variants.

kaleidawave · 2023-10-29T09:27:25Z

Hey, was away, so checking back in now.

This looks great!

Would be great to add positions to the rest of the Event variants. Unlike the return, they don't have any use in diagnostics/errors right now but will come in very useful in the future.

BTW if the conditional event variant is difficult then you can skip it.

wzwywx · 2023-10-29T10:54:36Z

Sure thing. I’ll have a look at the other Event variants as well and see what I can do. 29 Oct 2023 at 17:27 by ***@***.***:

…

Hey, was away, so checking back in now. This looks great! <https://user-images.githubusercontent.com/26967284/278861876-ad49cc8a-ae73-4788-b2e3-589b64fcc30b.png> Would be great to add positions to the rest of the > Event> variants. Unlike the return, they don't have any use in diagnostics/errors right now but will come in very useful in the future. BTW if the conditional event variant is difficult then you can skip it. — Reply to this email directly, > view it on GitHub <#69 (comment)>> , or > unsubscribe <https://github.com/notifications/unsubscribe-auth/A2ZOS3QPJIU7ZPAJ6JBAU6DYBYHPPAVCNFSM6AAAAAA6J7KLRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBUGA2DKOBSHE>> . You are receiving this because you authored the thread.> Message ID: > <kaleidawave/ezno/pull/69/c1784045829> @> github> .> com>

kaleidawave · 2023-11-01T13:56:32Z

Awesome, lmk the time frame you are thinking of. Also #71 changed a lot of files, so remember to update/fast-forward the branch before any changes

wzwywx · 2023-11-02T04:07:14Z

Thanks for the heads up on the change.

At the moment, no concrete time frame I'm afraid.

wzwywx · 2023-11-02T09:23:20Z

I'm working on the ReadsReference variant right now and after tracing all the errors once a position is added, I realised that the synthesise_class_fields function is linked to that event and can potentially pass more than one position i.e. one for each field.

ezno/checker/src/synthesis/expressions.rs

Lines 857 to 861 in 511e5e2

    
           pub(super) fn synthesise_class_fields<T: crate::ReadFromFS>( 
        
           	fields: Vec<(TypeId, PublicityKind, Expression)>, 
        
           	environment: &mut Environment, 
        
           	checking_data: &mut CheckingData<T, super::EznoParser>, 
        
           ) {

Should the event store the positions of all the fields?

kaleidawave · 2023-11-02T10:06:57Z

Ah interesting. Classes aren't fully implemented at the moment, I got 70% through then got stuck so there might be some unused non fully finished functions around. I would put a todo!("get event position") where the position is needed and I can figure out that part when I finish and fix classes.

wzwywx · 2023-11-02T10:23:02Z

Oh, got it. I’ll put a todo there then and move on to the next variant. 2 Nov 2023 at 18:07 by ***@***.***:

…

Ah interesting. Classes aren't fully implemented at the moment, I got 70% through then got stuck so there might be some unused non fully finished functions around. I would put a > todo!("get event position")> where the position is needed and I can figure out that part when I finish and fix classes. — Reply to this email directly, > view it on GitHub <#69 (comment)>> , or > unsubscribe <https://github.com/notifications/unsubscribe-auth/A2ZOS3XUGSIPXZFR7YDVX4TYCNWEXAVCNFSM6AAAAAA6J7KLRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJQGQZDQMJUG4>> . You are receiving this because you authored the thread.> Message ID: > <kaleidawave/ezno/pull/69/c1790428147> @> github> .> com>

kaleidawave

Looking great so far. So just Event::Setter left? Left a few minor suggestions around the diagnostic messaging. I have some good ideas for how to use these new getter and setter positions 👀 .

kaleidawave · 2023-11-03T11:51:18Z

checker/src/diagnostics.rs

 					reason: format!(
 						"Function is expected to return {expected_return_type} but returned {returned_type}",
 					),
+					labels: vec![(


Thinking about this, I think returned_position should be the main diagnostic position. Could we swap the values for the main position and label?

Then the main reason should be "Cannot return {returned type} from function expected to return {expected_return_type}" and the label one should be "function annotated to return {expected_return_type} here"

On it, let me swap them and see how the new message looks.

How does this look?

Yep perfect! Better than the TSC diagnostic 😉 https://www.typescriptlang.org/play?#code/GYVwdgxgLglg9mABADwBQEoBciwgLYBGApgE6IDeAsAFCJ2IlFQglIBEuhpbNAvkA

kaleidawave · 2023-11-03T11:52:10Z

checker/src/events/helpers.rs

@@ -55,8 +55,9 @@ pub(crate) fn get_return_from_events<'a, T: crate::ReadFromFS, M: crate::ASTImpl
 										&checking_data.types,
 										false,
 									),
-								position: annotation_span,
-								// TODO event position here #37
+								position: annotation_span.clone(),


Maybe rename position to annotation_position

kaleidawave · 2023-11-03T11:52:43Z

checker/src/events/helpers.rs

-								// TODO event position here #37
+								position: annotation_span.clone(),
+								// TODO test with return_position
+								returned_position: position.clone(), // TODO event position here #37


Looking perfect 🤩, can remove the // TODO #37 comment now!

wzwywx · 2023-11-04T06:55:04Z

Looking great so far. So just Event::Setter left? Left a few minor suggestions around the diagnostic messaging. I have some good ideas for how to use these new getter and setter positions 👀 .

I believe Setter, CallsType, Throw, Conditionally, and CreateObject are left.

Thanks for the suggestions! I'll look into those first before implementing the position for Setter.

Two positions - returned and annotation positions - are given their own names instead of a generic name of `position` for one of them

This implementation needs a closer look because adding a position to Event::Setter requires a lot more work to fix the resulting broken code compared to other events. The method `register_property` can optionally push the Event::Setter event provided that the argument `register_setter_event` is `true`. But because Event::Setter requires a position, it needs `register_property` to require a position as well as its parameter since the current parameters have no way to access position. This introduces certain problems because even though some properties needs `register_setter_event` to be true, they don't have a clear position that can be passed to Event::Setter. How should we handle the case when a property needs a setter event registered but has no position? For now, because of the existence of some properties with non-obvious (or perhaps absent) positions, the position in Event::Setter will be optional i.e. `Option<SpanWithSource>`.

kaleidawave · 2023-11-08T10:33:28Z

ignore the test issue, that is something I broke

Replying to setter position:

For now, because of the existence of some properties with non-obvious (or perhaps absent) positions,
the position in Event::Setter will be optional i.e. Option<SpanWithSource>

Yes that's perfect. The register_property function is a bit WIP and has some problems with it. If it is possible put a comment next to any Nones passed as positions to the setter and I will have a think if positions are even needed in that case.

wzwywx · 2023-11-08T11:29:17Z

Noted. I’ll have a look at what I can find and put a comment where there’s a None. I did put comments before some of the Nones, I think, but definitely not all of them. 8 Nov 2023 at 18:33 by ***@***.***:

…

ignore the test issue, that is something I broke Replying to setter position: > > > For now, because of the existence of some properties with non-obvious (or perhaps absent) positions, > the position in Event::Setter will be optional i.e. >> Option<SpanWithSource> > > > Yes that's perfect. The > register_property> function is a bit WIP and has some problems with it. If it is possible put a comment next to any > None> s passed as positions to the setter and I will have a think if positions are even needed in that case. — Reply to this email directly, > view it on GitHub <#69 (comment)>> , or > unsubscribe <https://github.com/notifications/unsubscribe-auth/A2ZOS3QI4D7OSGHA5RGYCSDYDNNYHAVCNFSM6AAAAAA6J7KLRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBRGU3TSNJRGI>> . You are receiving this because you authored the thread.> Message ID: > <kaleidawave/ezno/pull/69/c1801579512> @> github> .> com>

This implementation is partially in the sense that `Event::Conditionally` now has a position (albeit optional), but functions/methods that rely on the event only passes None as the position. There are two places where positions are unclear: 1. Else evaluation (`new_conditional_context()` - it might be possible to get a position from `SynthesisableConditional` but it does not have a `get_position` implemented yet 2. `create_this` - not sure if a position is needed here

Also incomplete because I'm not sure how to get a position from `func` (`SynthesisableFunction`). Every position is `None` for now.

wzwywx · 2023-11-11T04:26:03Z

I believe all event variants have positions now. Unfortunately, wiring up the positions for Setter, Conditionally, and CreateObject are difficult for me to implement so at the moment, most of them only have None.

kaleidawave · 2023-11-13T09:36:49Z

Awesome just adjusted to use the new return diagnostics message in the tests 071ce96

Apart from that all the other tests pass, so you haven't broken anything!!! and have just merged!

Thanks for the informative comments and commit messages. They will be useful for using the new positions on events.

wzwywx · 2023-11-13T09:39:24Z

Nice! Thank you for the merge.

I’m happy to have helped, and also thank you for your patience in guiding me throughout the whole process.

kaleidawave · 2023-11-13T09:42:31Z

Awesome. You are also the first to PR to the type checker part of Ezno and implemented something really deep in the core! If you have any feedback on what is messy, difficult or could be made clearer that would be really helpful for me to help future contributors!

Also might post this change on Twitter, let me know if/how you want a shout out :)

wzwywx · 2023-11-13T10:02:32Z

I have no particular feedback in mind, unfortunately. Though maybe, I’m not sure if it’s just me being new, but sometimes I was unclear whether certain part of the code is (relatively) complete or a work in progress. There are `todo!()` and `TODO` comment to indicate the WIP-ness, but I was oblivious to those so I chalked it up solely to my incompetence when I can’t make things work when in reality, it’s a mix of my lack of skill and working on incomplete code. 13 Nov 2023 at 17:42 by ***@***.***:

…

Awesome. You are also the first to PR to the type checker part of Ezno! If you have any feedback on what is messy, difficult or could be made clearer that would be really helpful for me to help future contributors! Also might post this change on Twitter, let me know if/how you want a shout out :) — Reply to this email directly, > view it on GitHub <#69 (comment)>> , or > unsubscribe <https://github.com/notifications/unsubscribe-auth/A2ZOS3V3GAFC4KF4F5WHXE3YEHTRDAVCNFSM6AAAAAA6J7KLRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBXG44TCNJXGI>> . You are receiving this because you authored the thread.> Message ID: > <kaleidawave/ezno/pull/69/c1807791572> @> github> .> com>

wzwywx marked this pull request as ready for review October 21, 2023 08:09

wzwywx marked this pull request as draft October 24, 2023 07:33

wzwywx marked this pull request as ready for review October 24, 2023 07:35

wzwywx added 3 commits November 2, 2023 15:47

First iteration

4e3bc18

Remove duplicate return position in diagnostics

de1fa94

feat:add position to ReadsReference variant

511e5e2

wzwywx force-pushed the feat-issue-37 branch from 2f391d5 to 511e5e2 Compare November 2, 2023 08:59

wzwywx added 3 commits November 3, 2023 13:09

wip:clarified incomplete position handling in synthesisable_class_fields

19fc945

feat:add position to SetsVariable event variant

3bd86c3

feat:add position to Getter event variant

d682aaa

kaleidawave reviewed Nov 3, 2023

View reviewed changes

wzwywx added 4 commits November 4, 2023 15:24

refactor: give clearer name to positions involved in Event::Return

ef88dbb

Two positions - returned and annotation positions - are given their own names instead of a generic name of `position` for one of them

cleanup: remove todos for issue 37

508f0be

Add position to Event::CallsType

fefa517

wzwywx added 2 commits November 9, 2023 16:06

Add position to Event::Throw

d34dd74

kaleidawave added checking Issues around checking diagnostics related to outputting code errors labels Nov 10, 2023

wip: Add position to Event::CreateObject

a57ba1b

Also incomplete because I'm not sure how to get a position from `func` (`SynthesisableFunction`). Every position is `None` for now.

kaleidawave added 2 commits November 13, 2023 08:51

Merge branch 'main' into feat-issue-37

0c16358

Update specification.md with the new diagnostics

071ce96

kaleidawave linked an issue Nov 13, 2023 that may be closed by this pull request

Add positions to events #37

Closed

kaleidawave merged commit 4bc8be5 into kaleidawave:main Nov 13, 2023
7 checks passed

kaleidawave mentioned this pull request Mar 9, 2024

Closure TDZ #124

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add positions to events (issue #37) #69

Add positions to events (issue #37) #69

wzwywx commented Oct 21, 2023 •

edited

Loading

kaleidawave commented Oct 29, 2023

wzwywx commented Oct 29, 2023 via email

kaleidawave commented Nov 1, 2023

wzwywx commented Nov 2, 2023

wzwywx commented Nov 2, 2023 •

edited

Loading

kaleidawave commented Nov 2, 2023

wzwywx commented Nov 2, 2023 via email

kaleidawave left a comment

kaleidawave Nov 3, 2023

wzwywx Nov 4, 2023

wzwywx Nov 4, 2023

kaleidawave Nov 6, 2023

kaleidawave Nov 3, 2023

kaleidawave Nov 3, 2023 •

edited

Loading

wzwywx commented Nov 4, 2023

kaleidawave commented Nov 8, 2023

wzwywx commented Nov 8, 2023 via email

wzwywx commented Nov 11, 2023

kaleidawave commented Nov 13, 2023

wzwywx commented Nov 13, 2023

kaleidawave commented Nov 13, 2023 •

edited

Loading

wzwywx commented Nov 13, 2023 via email •

edited

Loading

Add positions to events (issue #37) #69

Add positions to events (issue #37) #69

Conversation

wzwywx commented Oct 21, 2023 • edited Loading

kaleidawave commented Oct 29, 2023

wzwywx commented Oct 29, 2023 via email

kaleidawave commented Nov 1, 2023

wzwywx commented Nov 2, 2023

wzwywx commented Nov 2, 2023 • edited Loading

kaleidawave commented Nov 2, 2023

wzwywx commented Nov 2, 2023 via email

kaleidawave left a comment

Choose a reason for hiding this comment

kaleidawave Nov 3, 2023

Choose a reason for hiding this comment

wzwywx Nov 4, 2023

Choose a reason for hiding this comment

wzwywx Nov 4, 2023

Choose a reason for hiding this comment

kaleidawave Nov 6, 2023

Choose a reason for hiding this comment

kaleidawave Nov 3, 2023

Choose a reason for hiding this comment

kaleidawave Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

wzwywx commented Nov 4, 2023

kaleidawave commented Nov 8, 2023

wzwywx commented Nov 8, 2023 via email

wzwywx commented Nov 11, 2023

kaleidawave commented Nov 13, 2023

wzwywx commented Nov 13, 2023

kaleidawave commented Nov 13, 2023 • edited Loading

wzwywx commented Nov 13, 2023 via email • edited Loading

wzwywx commented Oct 21, 2023 •

edited

Loading

wzwywx commented Nov 2, 2023 •

edited

Loading

kaleidawave Nov 3, 2023 •

edited

Loading

kaleidawave commented Nov 13, 2023 •

edited

Loading

wzwywx commented Nov 13, 2023 via email •

edited

Loading