Skip to content

Async suggestions improvements#60528

Merged
CyrusNajmabadi merged 40 commits intodotnet:mainfrom
DoctorKrolic:async-suggestions-improvements
Apr 9, 2022
Merged

Async suggestions improvements#60528
CyrusNajmabadi merged 40 commits intodotnet:mainfrom
DoctorKrolic:async-suggestions-improvements

Conversation

@DoctorKrolic
Copy link
Copy Markdown
Contributor

@DoctorKrolic DoctorKrolic commented Apr 1, 2022

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:

    roslyn/src/Features/Core/Portable/Completion/CompletionServiceWithProviders.cs

    Line 301 in 65942dc

    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
Copy Markdown
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)

@runfoapp runfoapp bot mentioned this pull request Apr 4, 2022
Open
@DoctorKrolic
Copy link
Copy Markdown
Contributor Author

DoctorKrolic commented Apr 8, 2022

It seems you got a bit carried away)

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

CyrusNajmabadi commented Apr 8, 2022

It seems you got a bit carried away)

?

@DoctorKrolic
Copy link
Copy Markdown
Contributor Author

DoctorKrolic commented Apr 8, 2022

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

This was referenced Apr 8, 2022
Closed
Closed
Closed
@DoctorKrolic
Copy link
Copy Markdown
Contributor Author

DoctorKrolic commented Apr 9, 2022 •
edited
Loading

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
Copy Markdown
Contributor

CyrusNajmabadi commented Apr 9, 2022

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

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

@DoctorKrolic
Copy link
Copy Markdown
Contributor Author

DoctorKrolic commented Apr 9, 2022 •
edited
Loading

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
Copy Markdown
Contributor

CyrusNajmabadi commented Apr 9, 2022

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
Copy Markdown
Contributor

CyrusNajmabadi commented Apr 10, 2022

THanks!

@runfoapp runfoapp bot mentioned this pull request Apr 11, 2022
Closed
@dibarbet dibarbet modified the milestones: Next, 17.3.P1 Apr 25, 2022
@DoctorKrolic DoctorKrolic mentioned this pull request May 1, 2022
Merged
@DoctorKrolic DoctorKrolic mentioned this pull request May 16, 2022
Merged
@DoctorKrolic DoctorKrolic mentioned this pull request Jun 12, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@CyrusNajmabadi CyrusNajmabadi

Labels

Area-IDE

Projects

None yet

Milestone

17.3.P1

Development

Successfully merging this pull request may close these issues.

Odd behaviour with async method declaration: wrong types suggestion

5 participants

@DoctorKrolic @genlu @CyrusNajmabadi @dibarbet @jinujoseph