parseTreeToAst - issues: MutableList<Issue>

[hans-d]

Given

https://github.com/Strumenta/kolasu-kuki-languageserver/blob/ba34065fb9267bbb5904fe7801e9088759b77117/ast/src/main/kotlin/com/strumenta/kuki/parser/KukiKolasuParser.kt#L28-L32

Any reason why it is implemented in this way, with the "side effect" of mutating the issues parameter directly, instead of returning both the ast and the issues as a result of the call?

This seems intentionally, given the signature at

kolasu/core/src/main/kotlin/com/strumenta/kolasu/parsing/KolasuParser.kt

Lines 141 to 146 in 2114ed3

	protected abstract fun parseTreeToAst(
	parseTreeRoot: C,
	considerPosition: Boolean = true,
	issues: MutableList<Issue>,
	source: Source? = null
	): R?

(seems to be the same in 1.6)

[ftomassetti]

We eventually expose an object containing both the issues and the root of the AST (see

kolasu/core/src/main/kotlin/com/strumenta/kolasu/parsing/KolasuParser.kt

Lines 259 to 264 in 2114ed3

	fun parse(
	inputStream: CharStream,
	considerPosition: Boolean = true,
	measureLexingTime: Boolean = false,
	source: Source? = null
	): ParsingResult<R> {

).
However, to get to those results internally, we instantiate a list of issues once and pass it around to be populated from different steps, including this one

[hans-d]

The parseTreeToAst is less internal, as it needs to be implemented.

IMHO it would be cleaner (and clearer) to have the parseTreeToAst function just return the additional issues together with the AST (like parse), and let the caller handle the collection of all the issues instead of having to provide the results in two different ways.

[ftomassetti]

Thank you, we will take it into account for Kolasu 1.6

[ftomassetti]

This is now #337