Add positions to events about ezno HOT 10 CLOSED

Hi, @kaleidawave! Couple of questions on implementing this;

• Where will the position field from an Event be used? I noticed that the ReturnedTypeDoesNotMatch already has some position information and uses it while reporting, so I'm unclear on what this change would enable.
• Do you envision adding more metadata about Events in the future? Trying to evaluate whether it's worth defining a struct and adding that to each variant - seems premature at this stage vs. just directly adding position fields and refactoring later, if needed.
• The Event variants are mixed where most are struct-like, but some are not (e.g. SetsVariable and Throw). Is it worth refactoring to struct-like variants across the board or would adding an unnamed Span field be acceptable for those tuple-like variants?

Appreciate any and all guidance - thanks!

from ezno.

Hey @NitantP, thanks for the interest. Hopefully the following answers your questions

1. So currently the position field in ReturnedTypeDoesNotMatch just points to the return type annotation. What would be ideal is for that it to also contain the positions of the offending return statements. Getting these positions for these errors would be really simple if they were held on return events. This sort of thing is already implemented for AssignmentError::DoesNotMeetConstraint
   

   ezno/checker/src/context/mod.rs

   Lines 1390 to 1394 in 7cf648d

   	DoesNotMeetConstraint {
   	variable_type: TypeStringRepresentation,
   	variable_site: Span,
   	value_type: TypeStringRepresentation,
   	value_site: Span,

   so when it emits an error, it also adds a label (additional information) that outlines where the variable type annotation is
   

   ezno/checker/src/diagnostics.rs

   Lines 377 to 392 in 7cf648d

   	AssignmentError::DoesNotMeetConstraint {
   	variable_type,
   	variable_site,
   	value_type,
   	value_site,
   	} => Diagnostic::PositionWithAdditionLabels {
   	reason: format!(
   	"Type {value_type} is not assignable to type {variable_type}",
   	),
   	position: value_site,
   	labels: vec![(
   	format!("Variable declared with type {variable_type}"),
   	Some(variable_site),
   	)],
   	kind: super::DiagnosticKind::Error,
   	},

2. and 3. Can't think of any more metadata that is needed on Event rn, so just a Span on each should be fine. I see that SetsVariable and Throw are currently the only two unnamed variants, feel free to make the named variants like the others!

I am hoping it shouldn't be too difficult to start on, I am happy to help with any problems that arise if you decide to give it go! Note: you do not have to fully connect the errors up, I am thinking it might need another change for it to work perfectly. But if all you could get events setup with a position for each then it would be really useful for now and future ideas!

from ezno.

Just a heads up @NitantP. #61 changes the structure of positions a lot so might be worth waiting for that to merge before starting

from ezno.

@wzwywx awesome! Thanks for attempting it, if you do make any progress (working or not) would be great to put in a PR anyway as I am happy to help finish it if you make a start.

Might be worth reading the title 😅 but yes you have run into the exact problem!

What I would start with is adding a field to the return event variant, (events/mod.ts:68)

Return { 
  returned: TypeId,
  // \/ field you should add \/
  position: SpanWithSource
}

then you will see rust error a bunch and you just have to follow those until it compiles again (so you will probably have to do things like modify return_value (context/environment.rs:713) to take a position via a parameter and modify the synthesis code to give that position when it calls that).

Then once you have that field you can go back and get the diagnostic working!

And if you find that okay you can add a position field to every variant as I need it for other diagnostics 😀

Also no worries @NitantP

from ezno.

Hi @wzwywx - please feel free to submit a PR if you'd like. I haven't been able to make the progress I intended to given circumstances, so you may be in a better position to see this through.

Ah, I see. No worries @NitantP ; hopefully your circumstances are improving and I wish you well.

from ezno.

Understood - thanks, @kaleidawave!

from ezno.

Hi @NitantP #61 has been merged. So the repo code should be in a good state to work on this. Some updates:

• The positions should be source_map::SpanWithSource (aka have both a position and a file). This is the same as diagnostics/errors so there should be some good examples of creating them
• Since writing the issue I have reworked the return checking. (aka a function annotated as function x(): number, check that all returns are subtypes of number). Once events have positions, if you want to add the diagnostics you can check out

  ezno/checker/src/events/helpers.rs

  Line 17 in 88fc8e4

  pub(crate) fn get_return_from_events<'a, U: crate::FSResolver>(

  
  Specifically there is a place which would be ideal for using a position here
  

  ezno/checker/src/events/helpers.rs

  Line 58 in 88fc8e4

  // TODO event position here #37

LMK if there are any more questions :)

from ezno.

Hello, @kaleidawave. I'm trying to solve this issue on my end just to practice my Rust skills (no PR since I presume NitantP is still working on this issue.)

I understand how to modify the diagnostic messages and alter the ReturnedTypeDoesNotMatch to have the position for the return location.

But any thoughts on how to get the position of the location the value is returned?

I can see that get_return_from_events iterates through the events, but the pertinent event, Event::Return only stores a &TypeId and no position for when the event was triggered.

from ezno.

Hi @wzwywx - please feel free to submit a PR if you'd like. I haven't been able to make the progress I intended to given circumstances, so you may be in a better position to see this through.

from ezno.

from ezno.