Skip to content

TypeParser::parse() returns a tri-state (TypeNode|ParsedToken|null) that every caller re-decodes #59

Description

@MidnightDesign

Follow-up from code review of #58.

Problem

TypeParser::parse(Peekable $tokens): TypeNode|ParsedToken|null pushes the "was this actually a type?" decision onto every caller. The three-way return means each call site re-derives the same outcome and invents its own wording for the two failure shapes:

  • null → nothing was there (end of input)
  • ParsedToken → a token that can't start a type
  • TypeNode → success

There are 7 callers today, and 6 of them repeat this decode-and-throw dance with slightly different messages:

  • TypeParser::parseString()
  • TypeParser::parseFunction() (return type)
  • TypeParser::parseStruct() (field type)
  • ExpressionParser — inline variable type (foo:int)
  • ExpressionParser — lambda return-type annotation
  • E2eCase::parseTypes via the new parseDeclarations() (added in Make a string entry point consume its whole input #58)

The only caller that legitimately needs "maybe there's no type here" is parseTypeList(), which abuses the non-TypeNode return purely as a loop terminator.

That awkward return type is why the type-parsing call sites keep growing near-duplicate error branches — it was the root cause behind the E2eCase finding in #58.

Proposed direction

Make parse() throw SyntaxError on a missing/invalid type (it already throws via expect()) and return TypeNode unconditionally. Give parseTypeList() an explicit terminator (peek for the closing token / CloseAngle / CloseParen) instead of relying on a non-TypeNode return. That collapses roughly ten decode branches across the parser package into one convention.

This would also let the six callers drop their bespoke null/ParsedToken handling and just use the returned node.

Caveats

  • TypeParser is @psalm-internal, so this is not a public BC break — but it does change error messages. e.g. list<int 42> currently reports Expected >, got 42; after the change it would report something like Expected type, got 42. The message-based assertions in TypeParserTest and ExpressionParserTest will need updating, and we should decide which wording we actually want.
  • Worth doing as its own PR so the message churn is reviewable in isolation.

Related

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions