Skip to content

Conversation

@mlechu
Copy link
Collaborator

@mlechu mlechu commented Aug 5, 2025

The main part of #17:

As discussed, this conversion is a bit ugly, but necessary for backwards compatibility with existing macros.

Provenance is determined by using the latest seen LineNumberNode in the Expr traversal. I don't think the line number is well-defined for all subexpressions in general, but this method should work for the obvious cases (i.e. using a linenode that immediately precedes the current expr, or its parent if none, etc.)

TODO: This will need updates whenever we start using the latest dev version of JuliaSyntax

@c42f c42f force-pushed the caf/juliasyntax-dev-fixes branch from 63aa9b2 to 3514388 Compare August 5, 2025 02:04
Base automatically changed from caf/juliasyntax-dev-fixes to main August 5, 2025 11:04
@mlechu mlechu force-pushed the ec/expr-to-st branch 2 times, most recently from b678eef to 7cf9664 Compare August 5, 2025 23:27
@mlechu mlechu marked this pull request as ready for review August 5, 2025 23:29
@mlechu mlechu changed the base branch from main to ec/bugfix August 6, 2025 00:04
Base automatically changed from ec/bugfix to main August 7, 2025 15:22
@mlechu mlechu force-pushed the ec/expr-to-st branch 2 times, most recently from f5da731 to a3b60ed Compare August 12, 2025 16:57
Copy link
Owner

@c42f c42f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. There's a few things which can be refined (such as replacing @assert with something more graceful) but those can be done later.

One thing I noticed is that we need special provenance rules for Expr(:function) - the first LineNumberNode in the body block is the location where the function was defined. So function foo() end has provenance even though there's no place in the :function Expr for the line number. This is how Base.functionloc() works.

Also, could do with some light tests that basic line number provenance is working.

Copy link
Owner

@c42f c42f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, I think this is good to go :)

@mlechu mlechu merged commit 39b1eb4 into main Aug 15, 2025
1 check passed
@mlechu mlechu deleted the ec/expr-to-st branch August 15, 2025 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants