ast.DefinitionInfo should include the location of the source code

[DAddYE]

Hi!

Now each ast.Definition has func Info() DefinitionInfo() which returns a struct with Name and Line of the Definition, however, would be amazing if instead of the line we have a Location[int, int] (i.e token.Position) this would allow me to select back the text range in the source file and do extra parsing.

[abhinav]

Hey, it's not quite clear what you're asking. Did you want column numbers as
well? Or do you want the start and end positions?

The first would be possible by doing some extra book keeping in the lexer and
passing it on to the constructed nodes similar to the line number.

The latter is possible but significantly more difficult because it requires a
lot more changes to the parser and lexer.

If you expand on the use case a bit, we can maybe figure out a work around.

[DAddYE]

I answer on our chat, but for other readers, we need to store the start, and end position of the definition; which is line/column start and line/column ends.

[jparise]

@abhinav would you prefer to see this added as distinct Line and Column fields:

type DefinitionInfo struct {
	Name   string
	Line   int
    Column int
}

... or by moving to an embedded Position?

type DefinitionInfo struct {
    Position
	Name string
}

The first is the most backwards-compatible (and easier on the existing test literals). The latter simplifies construction and still allows .Line and .Column field access, although it is syntactically more verbose and breaks compatibility if people were expecting to be able to assign directly to .Line.

[abhinav]

Hey, sorry for the delay.
Switching to embedding might be considered a breaking change.

I would lean with the first option, but an alternative is a new Pos field, with Line deprecated in favor of it. We'd still fill Line for backwards compatibility, but Pos is what should be used going forward. Something like this?

type DefintiionInfo struct {
  Name string

  // Deprecated: Use Pos.
  Line int

  // Position of the definition in the source file.
  Pos Position
}

[jparise]

I think it will probably end up being more trouble maintaining both Line and Pos together. The parser tests will become pretty verbose.

We can always produce a Position from Line and Column fields (which I'll offer through a nodeWithPosition interface and an ast.Pos(ast.Node) function).

At a future point when we can break some backwards compatibility, we could drop Line and Column in favor of a single Pos, or decide to embed Position.

[abhinav]

Oh, that's reasonable. So to get a Position, you'd always call either the ast.Pos function or the Config.Pos method.
Yeah, in that case sticking the Column numbers directly on DefinitionInfo works.