refactor!: give expression parse failures a typed cause - #178
Merged
Conversation
parse_expression returned Option, so the reason a `${...}` did not parse
was worked out afterwards by explain_expression, which re-dispatched
through the parser's own predicates. The two could disagree, and a
consumer could only tell an unknown namespace from a malformed name by
matching on message text.
parse_expression_detailed returns Result<Expression, ExprError> and
parse_expression is its .ok(). ResolveError::InvalidReferenceSyntax
carries the ExprError. The explain_* family is deleted.
A source is dispatched on its shape before anything is parsed, so a
failure reports what the author reached for rather than the last of three
alternatives to fail. This also fixes a panic: a lone quote satisfied
both ends of the old literal check and sliced `&s[1..0]`, crashing `rite
check` and the language server. It was reachable from an ordinary value,
since a comma inside a string splits the argument list: `${concat(",",
param.a)}` was enough.
Diagnostics gained what the typed cause makes cheap. An unknown namespace
within two edits of a real one is named, so `${paramm.x}` suggests
`param`. A failing function argument reports its position and its own
cause. Text that opens with `${` and never closes reports Unclosed, where
it used to read as advice for text that never opened one. A pipeline with
nothing before its first `|` reports MissingSource rather than claiming
the expression is empty. The material guidance matches within one edit,
so the plural that names the declaring key lands on it too, and
suggestion() offers the artifact namespace as the fix.
One splitter serves both delimiters the grammar has, and one call parser
serves both positions a call appears in. parse_ref takes the text already
split at its first dot, so the unreachable missing-namespace arm and the
second scan for that dot both go away, along with a Vec per reference on
the parse path. Empty has one owner instead of three. RefType::from_str
reads the namespace spellings off ALL rather than repeating them.
Three failures that were riding InvalidReferenceSyntax as hand-written
strings get their own variants: ExpectedReference for a pipeline where
one reference belongs, and ReadsInputNotAString and
ReadsNotAReferenceOrMap for a `reads:` value of the wrong YAML type.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
parse_expressionreturnedOption, soexplain_expressionworked the reason out afterwards by re-dispatching through the parser's own predicates. The two could drift, and had.parse_expression_detailedreturnsResult<Expression, ExprError>;parse_expressionis its.ok(), so callers are untouched. Theexplain_*family is deleted. A source is dispatched on its shape before anything is parsed, so a failure names what the author reached for.Fixes a panic on
main: a lone quote satisfied both ends of the old literal check and sliced&s[1..0].${concat(",", param.a)}is enough to hit it, since the comma inside the string splits the argument list, and it takesrite-lsdown on a keystroke.Diagnostics gained near-miss namespaces (
${paramm.x}suggestsparam), argument positions, and separate causes for unclosed text and a missing pipeline source.Breaking:
ResolveErrorgains three variants,InvalidReferenceSyntax.reasonis nowExprError, andexplain_expressionis removed.Closes #173