Fix functionloc

[c42f]

This is a WIP of one way to fix #34820 — I'm not sure it's the best approach, but throwing it up there for discussion.

The approach taken here is to add a line number node to the method signature argument in Expr(:method, name, signature, body) after the signature argument types and names. I tried several options involving lowering of Expr(:method) and it turns out that this is quite minimally invasive way to do this. I also think it makes some sense: the method declaration is more a property of the method than it is relevant to the body CodeInfo.

However, there's one downside I know about, which is that functions defined in expressions (rather than as statements) can have a line number which is incorrect. In particular, the following test breaks:

let f(x) =
      g(x) = 1
    @test_broken functionloc(f(1))[2] > functionloc(f)[2]
end

Presumably this could be fixed by changing parsing of functions which occur inside expressions to be enclosed in an Expr(:block). Perhaps that's a reasonable way to go?

TODO

• Add extra line number node to long form function bodies during parsing, and extract that in lowering.
• @KristofferC mentions that Line number for first statement(s) in short form of function definition is wrong #31458 is related — investigate whether we can fix that as part of this work. (Decision: not directly related - fix separately)

[JeffBezanson]

Ah, interesting. f(x) = x syntax has the opposite problem: it only records the line of the signature and not the first body expression. So indeed it seems a comprehensive solution requires modifying the parser. I think adding a block expression around every function is too disruptive; it would be easier for the parser to record two locations at the start of each function body, and have lowering move the first one into the signature field of method exprs as you have here.

[c42f]

Ok, the doctests bring up some more serious ways that this approach can lead to unfortunate results:

• Ignoring the body leads to surprises when eval()ing ASTs containing function definitions which are not embedded in a block. This is hard to fix because in the current AST there's just nowhere natural to stash source location of a bare function definition unless we store it in the body statements. Wrapping in a block seems really disruptive.
• It seems that source location tracking is broken for inner constructors because structs ASTs have special case traversal in expand-forms. This can be fixed, but leads me to think location tracking within lowering is too fragile for this use.

In general I like this approach on the runtime side but it seems too fragile to use with confidence without parser support.

I think adding a block expression around every function is too disruptive; it would be easier for the parser to record two locations at the start of each function body, and have lowering move the first one into the signature field of method exprs as you have here.

Absolutely agreed! Let's do it this way.

[JeffBezanson]

LGTM, just has the inevitable detailed test errors.

[c42f]

Thanks for taking another look, I'll sort out those test issues, then merge.

I think this is likely to incidentally break the occasional package even though it's technically-not-API-breaking. Is there any way to flag this kind of thing so users can easily understand what has changed and how to fix their code? Applying the breaking label seems like an overstatement.

[c42f]

I felt like using set-cdr! here wasn't great. Is there a more natural idiomatic way to do this?

[c42f]

(Taking into account that we'll need to return both the line number and the body if this is pure.)

[c42f]

Ok, so this should be very close. There's one remaining problem: code coverage has changed because it now includes the line of the function definition. It turns out this is because the line from the jl_method_t is used in codegen for coverage:

julia/src/codegen.cpp

Line 5070 in f4c3f1d

toplineno = lam->def.method->line;

julia/src/codegen.cpp

Line 5910 in f4c3f1d

coverageVisitLine(ctx, ctx.file, toplineno);

I'm not sure what the desired outcome would be here — is it useful to consider the function definition line as code which should be included in the coverage output, or not?

There's also other uses of toplineno related to debug info. From what I can see these relate to emitting debug info for the function and function arguments so using the true function line number should be fine for these I guess.

[c42f]

To record a discussion about coverage with Jeff and Keno - the consensus was that we shouldn't visit the function definition line (unless it contains executable code), so some small adjustments to coverage codegen will be needed here.

[c42f]

Some code archeology on the special case handling of toplineno in coverage generation:

• It was needed due to bd06d27 (maybe the change at line

  julia/src/alloc.c

  Line 560 in bd06d27

  body[j] = jl_nothing;

  )
• Which led to the bug 0.5 code-coverage regression #17442
• Which was fixed in Fix coverage for the first statement in the function #17470

I think the code which the toplineno workaround related to has since been replace by the line table and having linearization as part of lowering, so should be possible to revert #17470 now.

[c42f]

I've removed this coverage stuff for the reasons explained in the commit message: It seems like a workaround for an old bug #17442, and now we have this double-counting workaround which appears to be a workaround-workaround. So I just deleted this. I also found coverageVisitStmt a bit confusing so I added some comments above, which I hope correctly state what is going on.