Skip to content

Conversation

@sandersn
Copy link
Member

Fixes #34 parsing of OptionalTypeNode. Previously, T? was always parsed as JSDocNullableType and then parseTupleElementType would throw away the nullable type node and construct a OptionalTypeNode. Now T? is always an optional type. The code is slightly simpler.

I didn't change the way it's parsed, even though it's not really in an obvious location. I really wanted to parse T? only in tuple position, inside parseTupleElementType, but by that point T? has already been parsed as the start of a malformed conditional type. There might be a way to move the construction of OptionalTypeNode to conditional type parsing, but that's not much better than where it is now.

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

Seems fine to me but I think the title isn't correct as we didn't actually back out JSDoc.

@sandersn sandersn changed the title Reinstate JSDoc ast Completely replace JSDocNonNullableType parsing with OptionalType Nov 22, 2024
@sandersn sandersn merged commit 4a51ebc into microsoft:main Nov 22, 2024
11 checks passed
@sandersn sandersn deleted the reinstate-jsdoc-ast branch November 22, 2024 16:37
@rbuckton
Copy link

rbuckton commented Nov 22, 2024

Does allow us to illegally parse T? in any type position? I had a different fix in the pretty printer work that only parsed ? when parsing a tuple element

@sandersn
Copy link
Member Author

It will indeed parse T? in any type position.

That's the same language as tsc, except that all postfix JSDocNonNullableTypes are now OptionalTypes.

It would be better to only parse in tuple context. If not, we'll need to update the grammar check to error on OptionalTypeNode outside a tuple type.

@ahejlsberg
Copy link
Member

ahejlsberg commented Nov 23, 2024

Seems to me the simplest solution is to always parse out OptionalTypeNode and then have a grammar check that forbids them anywhere but as a child of a tuple type node. That's very much in keeping with our philosophy of parsing out a more permissive superset (to handle common user errors) and then erroring in the grammar check.

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.

4 participants