MichaelHatherly / CommonMark.jl

A CommonMark-compliant Markdown parser for Julia.
Other
84 stars 11 forks source link

Fix interpolation with repeating, mutating names #51

Closed mortenpi closed 1 year ago

mortenpi commented 1 year ago

A variable that is interpolated multiple times with updates in between will print with the final value in all positions:

julia> let x = 1
           function f!(); x += 1; 42; end
           cm"$(x), $(f!()), $(x)"
       end
 2, 42, 2

This is because JuliaValue is a struct and therefore the two instance of JuliaValue(:x, nothing) are identical. This confuses the replacement Dict in _interp!, where value from the earlier position gets overwritten.

Changing JuliaValue to be mutable and using IdDict in _interp! fixes this:

julia> let x = 1
           function f!(); x += 1; 42; end
           cm"$(x), $(f!()), $(x)"
       end
 1, 42, 2

A few notes:

MichaelHatherly commented 1 year ago

Nice catch.

An alternative solution could be to add a position counter to JuliaValue, which would get incremented every time the parser constructs a new JuliaValue.

I think I'm more partial to a counter-based approach to this rather than making JuliaValue mutable, but if that implementation would be much more complex than the current one then we can just go with this IdDict approach.

mortenpi commented 1 year ago

I've updated this with a counter-based approach. This actually allows us to get rid of the Dict altogether, since we're literally storing the array indices in the counter now.

However, rather than just adding the .pos field to JuliaValue, I decided to create a new JuliaExpression inline element that gets added to the tree during parsing, and then replaced by JuliaValue. This has two benefits in my mind:

  1. It allows us to discard the position information, since that becomes irrelevant once the values have been evaluated.
  2. It makes it explicit whether it's an expression that has been evaluated or not (in case someone runs the interpolation parser without the cm"" macro). Because of this, I also removed the one-argument constructor for JuliaValue, implying that a JuliaValue must always be an evaluated value.
mortenpi commented 1 year ago

Added the writer methods.

MichaelHatherly commented 1 year ago

Thanks Morten!