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

[JavaScript] TypeScript cast throws off further highlighting #3862

Closed
rchl opened this issue Oct 30, 2023 · 8 comments · Fixed by #3863
Closed

[JavaScript] TypeScript cast throws off further highlighting #3862

rchl opened this issue Oct 30, 2023 · 8 comments · Fixed by #3863

Comments

@rchl
Copy link
Contributor

rchl commented Oct 30, 2023

What happened?

This probably can be simplified quite a bit.

Things already seem wrongly colored on the two method, after the cast.

const x = {
    one() {
        return foo as number < 3;
    },
    two() {
        this.update({
            bar: true,
        });
    },
}

Screenshot 2023-10-30 at 15 29 02

@Thom1729
Copy link
Collaborator

Ugh. It's surely parsing this as foo as (number < 3; …, not (foo as number) < 3;. I'm playing around with it in AST Explorer and apparently TypeScript parses number<3 differently from Foo<3, which is annoying (and, of course, totally undocumented). I guess I need to special-case the builtin types so that they don't try to consume generic parameters.

@rchl
Copy link
Contributor Author

rchl commented Oct 30, 2023

Not sure if I understand how you intend to fix it but note that instead of number one could use Foo where Foo is type Foo = number.

@Thom1729
Copy link
Collaborator

I don't think x as Foo < 3; is valid. In AST Explorer, it parses all of Foo < 3 as a type reference (which is invalid because it's missing the closing >). So I think that it will always treat the < as the beginning of type arguments after a general identifier, and never after a special identifier like number.

The solution I'm trying is removing the type argument handling from ts-type-expression-end and putting it in ts-type-basic instead. That passes all of the current tests, and fixes x as number < 3. I'm currently digging around for other corner cases to look at.

@Thom1729
Copy link
Collaborator

N.B. While I think that x as Foo < 3; is invalid, it does break rather ungracefully, and this is probably worth improving as well. Probably by telling the type contexts to bail if they see a semicolon, or somesuch.

@rchl
Copy link
Contributor Author

rchl commented Oct 30, 2023

I don't think x as Foo < 3; is valid.

you're right

@Thom1729
Copy link
Collaborator

Should be fixed in the linked PR.

@rchl
Copy link
Contributor Author

rchl commented Oct 30, 2023

Probably by telling the type contexts to bail if they see a semicolon, or somesuch.

Note that the community likes to omit semicolons for the sake of cleaner code. So that... :)

@Thom1729
Copy link
Collaborator

Eh, only so much we can do in such cases. For one thing, there's no way to detect EOF, so it's almost impossible to handle some otherwise-ambiguous constructs right before EOF.

Here, it's invalid syntax either way, so highlighting is on a best-effort basis. I didn't bother addressing that case in the linked PR because the solution for the valid case turned out to be simple. I did note that VSCode's fails in the same way in the invalid case, for what that's worth.

deathaxe pushed a commit that referenced this issue Oct 31, 2023
Fix #3862.

Originally, type arguments were part of the regular type expression
structure, so the syntax would try to highlight type arguments after
any type. But actually, only ordinary named types can get them. In the
linked issue, this was causing problems with `as`-style type
assertions — when followed by a `<`, the type expression was eating it
and trying to parse type arguments, even when the preceding type does
not accept them.

According to the spec — I'm kidding, we all know the drill by now,
actually after a bunch of futzing around in AST Explorer — this should
work correctly in all cases I can think of.

In order for this to work in all cases, it's necessary to ensure that
all specially keyworded types are accounted for, so this PR also adds
the `true` and `false` singleton types.
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 a pull request may close this issue.

2 participants