Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

type identifier should not trigger an parsing error when part of an assertion #58712

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

jean-michelet
Copy link
Contributor

@jean-michelet jean-michelet commented May 30, 2024

Will fixes #58248

I'm not sure if there are any other cases I've missed. I'll take a closer look this week, but feel free to give feedbacks 🙏

This should be ok:

const type = 'x';

type satisfies string;
type as string;

// EDIT: the following example is obsolete
// const interface = 'x';
// interface satisfies string;
// interface as string;

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label May 30, 2024
@whzx5byb
Copy link

whzx5byb commented May 30, 2024

interface satisfies { } will be ambiguous: Does it define an empty interface named satisfies? Or does it validate that the type of a variable named interface matches {}?

Also, considering interface is already a reserved word in strict mode, I don't think it's necessary to eliminate the parsing error for interface.

@jean-michelet
Copy link
Contributor Author

interface satisfies { } will be ambiguous:

Oops, indeed.

@jean-michelet jean-michelet changed the title (type | interface) as part of an assertion should not trigger an parsing error. type identifier should not trigger an parsing error when part of an assertion May 30, 2024
Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

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

The more I think about it, I realized this is a bit of a tricky problem because the look-ahead is unbounded. You can write

type satisfies <T>() => T

And from the parser's point of view, when it sees

type satisfies <

it needs to make a call of whether this is going to be a type alias or a satisfies expression.

So what I would do is keep the special-casing for as and satisfies and make sure we have tests for things like

// These were written with the expectation of parse errors.
// We expect the parser to try constructing a statement expression
// with 'satisfies' and 'as' expressions.

type satisfies<T> = T;
type as<T> = T;

and

// These should be parsed as statements
// with 'satisfies' and 'as' expressions.

type satisfies <T>() => T;
type as <T>() => T;

}

switch (token()) {
case SyntaxKind.AsKeyword:
Copy link
Member

Choose a reason for hiding this comment

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

It really feels like this function should have been left as-is, and the look-ahead should have been done after calling this function specifically for the type keyword.

It also might be the case that you don't actually need to special-case as and satisfies, you can just do an unconditional look-ahead. But I might have to think about that.

switch (token()) {
case SyntaxKind.AsKeyword:
case SyntaxKind.SatisfiesKeyword: {
return lookAhead(() => nextToken() === SyntaxKind.EqualsToken);
Copy link
Member

Choose a reason for hiding this comment

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

The name of a type alias isn't always immediately followed by an =, so I don't think this specific look-ahead is enough.

See the comment I'm leaving in my review.

type satisfies<T> = T;

@DanielRosenwasser
Copy link
Member

Also, like @whzx5byb mentioned, you should add an interface satisfies/interface as test to ensure there's no regressions

Copy link
Contributor Author

@jean-michelet jean-michelet left a comment

Choose a reason for hiding this comment

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

I hope I understand what you have in mind.
I've left comments to make it clear what I understand needs to be done.

~~~~~~
!!! error TS1005: '{' expected.
~~~~~~
!!! error TS2693: 'string' only refers to a type, but is being used as a value here.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sure there are no regressions concerning the interface keyword.

Copy link
Member

Choose a reason for hiding this comment

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

Can you explicitly test interface satisfies { }?

@@ -0,0 +1,2 @@
type satisfies<T> = T;
type as<T> = T;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type parameters were not parsed.
This test ensures these are considered type aliases.

const type = <T,>(x: T) => x;

type satisfies <T>(x: T) => T;
type as <T>() => T;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sure that assertions with type parameters are parsed as assertions.

return nextTokenIsIdentifierOnSameLine();
case SyntaxKind.TypeKeyword:
const isIdentifierOnSameLine = nextTokenIsIdentifierOnSameLine();
if (isIdentifierOnSameLine && token() === SyntaxKind.AsKeyword || token() === SyntaxKind.SatisfiesKeyword) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think this part should be in a proper function?
We could return something like this:

return nextTokenIsIdentifierOnSameLine() && someNameToExplainsWhatIsDoneRegardingSatisfiesAndAs()

if (isIdentifierOnSameLine && token() === SyntaxKind.AsKeyword || token() === SyntaxKind.SatisfiesKeyword) {
return lookAhead(() => {
nextToken();
parseTypeParameters();
Copy link
Member

Choose a reason for hiding this comment

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

This feels like it would be pretty wasteful, but I suppose that people can't write this code today anyway and it'd only be paid for as and satisfies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

satisfies does not work on a variable called type
5 participants