Fix Expr conversion of erroneous operator dot call

[Liozou]

Fix #341

The root issue was the unconditional conversion of an Expr(:dotcall, foo, ...) to Expr(:call, Symbol(:., foo), ...) when foo was not an Expr(:., ...). That case corresponds to dotted operators (where foo is the symbol of the operator), but it can actually also occur when there is a parsing error, such as misplacing a non-unitary dotted operator like in the MWE.

I'm not completely sure what Expr should be yielded here, I went for Expr(:dotcall, Expr(:error, Expr(:., foo), ...) because it required minimal change, but let me know if it should be something else!

[codecov]

Codecov Report

Merging #374 (66f7eba) into main (1b048aa) will increase coverage by 0.00%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #374   +/-   ##
=======================================
  Coverage   96.58%   96.58%           
=======================================
  Files          14       14           
  Lines        4160     4161    +1     
=======================================
+ Hits         4018     4019    +1     
  Misses        142      142           

Files	Coverage Δ
src/expr.jl	99.69% <100.00%> (+<0.01%)	⬆️

... and 3 files with indirect coverage changes

[c42f]

Awesome, thanks for the PR!

As for the appropriate Expr here, we currently have the following conversion for .+ (which of course is valid):

julia> parsestmt(SyntaxNode, ".+julia", ignore_errors=true)
line:col│ tree                                   │ file_name
   1:1  │[dotcall-pre]
   1:2  │  +
   1:3  │  julia


julia> dump(Expr(parsestmt(SyntaxNode, ".+julia", ignore_errors=true)))
Expr
  head: Symbol call
  args: Array{Any}((2,))
    1: Symbol .+
    2: Symbol julia

And we have the following tree for ./ which contains the error

julia> parsestmt(SyntaxNode, "./julia", ignore_errors=true)
line:col│ tree                                   │ file_name
   1:1  │[dotcall-pre]
   1:1  │  [error]
   1:1  │    [.]
   1:2  │      /
   1:3  │  julia

Expr(:dotcall) isn't something which consumers of Expr know about - they only know about Expr(:call) so I think we should go with the closest analogy using Expr(:call) that we can find. This should probably be Expr(:call, Expr(:error, Expr(:., :/)), :julia) ?

[Liozou]

That's perfectly sensible: done in bea7bae, thanks for the correction!

Another question: should I assert that startsym is either a Symbol or an Expr(:error, ...) at the following:

JuliaSyntax.jl/src/expr.jl

Lines 276 to 278 in bea7bae

	if startsym isa Symbol
	args[1] = Symbol(:., startsym)
	end # else startsym could be an Expr(:error), just propagate it

i.e. change the comment into an else clause that errors if startsym is not an Expr(:error, ...)? I don't know if the current policy is to eagerly error when encountering unexpected expressions at that point of the parser, or letting them flow.

[c42f]

Thanks! I've noted a couple more minor quibbles, but I think this is basically good to go.

I'm in two minds about asserting that the else should be an Expr(:error). On the one hand, the parser shouldn't create any other forms. But on the other hand, people can construct SyntaxNodes programmatically and it might make sense just to convert them rather than throw. I don't really know what's best. Maybe leave as-is :-)

[Liozou]

Thanks for the review, much better now!
I had somewhat similar hesitations regarding the error behaviour... But in any case this can be handled in a later PR if need be, it is not directly related to this bugfix anyway.

[c42f]

Looks lovely, thank youu :)

[Liozou]

Thank you for all the help!