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[bot]]

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%)	:arrow_up:

... 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 https://github.com/JuliaLang/JuliaSyntax.jl/pull/374/commits/bea7bae2f3d6ff29ff1e820f1efbd832e714f0ea, thanks for the correction!

Another question: should I assert that startsym is either a Symbol or an Expr(:error, ...) at the following: https://github.com/JuliaLang/JuliaSyntax.jl/blob/bea7bae2f3d6ff29ff1e820f1efbd832e714f0ea/src/expr.jl#L276-L278 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.

[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!