Skip to content

Conversation

@DoctorKrolic
Copy link
Contributor

Fixes: #60341

This is my largest PR so far)

Current result for C#:
devenv_91GD0mhYi0
A bit of overflown by snippets, but it will be fixed when semantic snippets will come into play, I guess

Known issues:

  • Test project for completions targets net472, which disables my ability to test ValueTask and, what is more important IMO, IAsyncEnumerable and IAsyncEnumerator as special cases
  • For some reason AsyncMethodBuilder is not available in System.Runtime.CompilerServices when running tests, so I can't test the case with user defined awaitable. I guess, this is the because of same reason as an issue above, but I am not sure here
  • Preselection is different in different situations:
    devenv_e4e5m9Fm0c
    Note how preselection in the second case is kind of unfocused, so if I want to navigate via arrows on keybord, I need to press an arrow twice: first time to make completion window focused, and second time to actually navigate. This doesn't happen in the first case
    I tried to place breakpoint here to observe selection behaviour in both situations:
    return completionContexts.Where(HasAnyItems).ToImmutableArray();

    ...And in both cases all items in all contexts had Default behaviour, while Task had HardSelection, which is expected here. So I don't know, why preselections are different then
  • Preselection does not work in some scenarios when typing:
    devenv_qCVuXWHeWQ
    This thing is hard to properly debug, because completions are shown whenever you type any charachter. So I currenly don't know the cause of this bug either

I encourage to review this commit by commit. Also would like some hints of where can possibly causes of preselection bugs be (at least where should I look first) and what should I do to properly test my changes.

@DoctorKrolic DoctorKrolic requested a review from a team as a code owner April 1, 2022 21:27
@ghost ghost added Community The pull request was submitted by a contributor who is not a Microsoft employee. Area-IDE and removed Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Apr 1, 2022
@genlu
Copy link
Member

genlu commented Apr 1, 2022

Preselection is different in different situations

I haven't looked closely yet, but the fact that in your screen capture, the first async is classified but the second has squiggle means the parser parsed the async as an identifier (i.e. a Type name) until something is typed after that, which is not unreasonable considering async is a contextual keyword. For example, no one can stop you from doing something like this :)

public class async
{
}

public class B
{
    public async Bar() => null;
}

This would also explain the soft-selection behavior: completion automatically enters "suggestion mode" when it thinks it's in the name context, (in this case, since async is parsed as a type, Roslyn is anticipating user to name something of that type)

@DoctorKrolic
Copy link
Contributor Author

It seems you got a bit carried away)

@CyrusNajmabadi
Copy link
Member

It seems you got a bit carried away)

?

@DoctorKrolic
Copy link
Contributor Author

I mean, I had originaly only 50 files changed. Now there are 95

@DoctorKrolic
Copy link
Contributor Author

DoctorKrolic commented Apr 9, 2022

Completions for VB are not fully correct:
devenv_b633oVmgu9
VB supports only Task or Task(Of ...) return type for Async methods. So there is no point in having other awaitable types, IAsyncEnumerable, IAsyncEnumerator, namespaces and Global keyword completions there

And preselected item is soft-selected for some reason (this is good to fix in a follow-up PR)

@CyrusNajmabadi
Copy link
Member

You can say global.System. Threading. Tasks.Task

But honestly, I think this is splitting hairs a bit too much.

@DoctorKrolic
Copy link
Contributor Author

DoctorKrolic commented Apr 9, 2022

You can say global.System. Threading. Tasks.Task

Ok. Global and System (or even all namespaces) can stay

But honestly, I think this is splitting hairs a bit too much.

Why? if we are handling this case, why not to implement it properly? Initially I had a distinction between C# and VB, so VB had only Global, Task and Task(Of ...). Now it seems for me to be a regression.

@CyrusNajmabadi
Copy link
Member

Taking to discord to discuss :-)

@CyrusNajmabadi CyrusNajmabadi merged commit beb6f39 into dotnet:main Apr 9, 2022
@ghost ghost added this to the Next milestone Apr 9, 2022
@DoctorKrolic DoctorKrolic deleted the async-suggestions-improvements branch April 9, 2022 19:32
@CyrusNajmabadi
Copy link
Member

THanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Odd behaviour with async method declaration: wrong types suggestion

4 participants