Colorize async as keyword in some cases

[DoctorKrolic]

Fixes: #60339

[Youssef1313]

I'm concerned about this classifying async as keyword in cases where it doesn't.

[DoctorKrolic]

@Youssef1313 I classify it as a keyword only if there is no type with async name is defined. Without my changes it is just pure white, so I think it is better to colorize it, because using async as a keyword is a lot more common, than using async as a type

[Youssef1313]

@Youssef1313 I classify it as a keyword only if there is no type with async name is defined. Without my changes it is just pure white, so I think it is better to colorize it, because using async as a keyword is a lot more common, than using async as a type

Does it get colorized for things like public C.async (just an example)? async here can never be a keyword. It doesn't, the parent is properly checked. But I'm not sure if there are some edge cases.

[sharwell]

If we colorize async token as a keyword and then colorize over the top as a type during the semantic pass, will it consistently show up as a type? If so, it seems to make sense to colorize the token during the syntax pass as a keyword since that's most likely correct.

[DoctorKrolic]

@sharwell Consider this piece of code: public async Foo { get; set; }. Even if async type is not defined, it can never be a keyword here, so in this situation it is better to leave it blank. I added a bunch of tests with such examples

[CyrusNajmabadi]

If it doesn't bind as a symbol it is either the 'async' keyword, or it's an error case. In each case colorizing as a keyword is fine.

[DoctorKrolic]

In each case colorizing as a keyword is fine.

I gave an opposite example above: public async Foo { get; set; }. If we colorize async as a keyword here, it can be confusing to the user, because if he had written so, then he knew, what he is doing. Maybe he wanted to add async type via codefix later. Or I don't get, what you are trying to say?

[CyrusNajmabadi]

I gave an opposite example above: public async Foo { get; set; }. If we colorize async as a keyword here, it can be confusing to the user, because if he had written so, then he knew, what he is doing. Maybe he wanted to add async type via codefix later. Or I don't get, what you are trying to say?

It's code in error, i have no problem with code in error having some behavior that might be confusing to one person but not to another. For example, in reality it's much more likely that hte user intended to add Task after async instead of defining an async type. So coloring as a keyword is entirely appropriate.

Think about it this way. In literally 99.999% of these cases, it's going to be the keyword. So we have our code fallover to that in an error condition. For nearly all users that will be ok. The only user that will have an issue is hte 1 in a million user who actually defines a type called 'async'.

From the IDE perspective, we do not cater to that user. They are already doing something well outside the norm and the behavior (in error conditions) should not consider their needs over the needs of everyone else who is doing things the normal way.

[DoctorKrolic]

@CyrusNajmabadi I have already implemented logic, that covers all currently known incomplete member cases. I think, your advice could be perfectly fine, if I knew it before touching the code. But should I reimplement it now, when all known cases are handled and tested, really? It doesn't seem to have any performance impact or somthing like this. It seems for me to be a downgrade. When I tried to do this on compiler layer, you said, that it is a breaking change and a lot of possible risks. Now you are telling me, that IDE can ignore users, for whome this could be a breaking change just because there are vanishingly few of those.

[CyrusNajmabadi]

I think, your advice could be perfectly fine, if I knew it before touching the code.

Sure. I recommend in the future potentially running ideas past us to ensure that we're all in alignment on what to do.

But should I reimplement it now,

You don't have to reimplement. I'm willing to do it for you.

when all known cases are handled and tested, really?

Yes. Fundamentally, i would not be ok with a policy of: the code was written in a non-ideal way, but let's take it. We should code as per the designs and principles we have, and keep the codebase clean wrt to that. I do think this is painful since you've done the work, so i'm happy to take my time to revise the PR to get things there so you don't have to do that yourself. In the future though, we can likely avoid such back/forth with some up front discussions on the topic :)

It doesn't seem to have any performance impact or somthing like this. It seems for me to be a downgrade.

I don't believe it's a downgrade. As i said, for 99.999% of users it's the better behavior.

When I tried to do this on compiler layer, you said, that it is a breaking change and a lot of possible risks.

Yes. That's the compiler. It has entirely different compat/maintainability concerns. The IDE follows entirely different rules and beliefs here precisely because we are not beholden to things like this with stuff like keyword classification.

Now you are telling me, that IDE can ignore users,

Yes. And that's the normal case for us. We have limited resources and we invest in the 99.99% case so we deliver more value to the majority of the ecosystem versus expending extra effort for outliers. We think of this as the right investment of our resources.

[CyrusNajmabadi]

@DoctorKrolic are you ok with me pushing to your branch?

[CyrusNajmabadi]

Also, if you're available, i'd love to chat on discord. You can reach me at discord.gg/csharp, using the alias "Metasyntactic". Thanks!

[DoctorKrolic]

Yes, it's absolutely fine

[CyrusNajmabadi]

Working on this now.

[DoctorKrolic]

Just interesting: what is main-vs-deps branch for?

[CyrusNajmabadi]

Thanks!

[CyrusNajmabadi]

It's where a lot of feature work has been done for the 17.3 release. I'd prefer your work go in there and not in the main branch so we don't have conflicts to deal when we merge main-vs-deps into main.