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

Fix completions in range expressions #66959

Merged

Conversation

DoctorKrolic
Copy link
Contributor

Fixes: #66903

@DoctorKrolic DoctorKrolic requested a review from a team as a code owner February 19, 2023 18:29
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Feb 19, 2023
@DoctorKrolic DoctorKrolic force-pushed the completions-in-range-expression branch from c03e0ef to 51337b9 Compare February 19, 2023 18:55
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

Thanks!

@DoctorKrolic
Copy link
Contributor Author

DoctorKrolic commented Feb 19, 2023

@CyrusNajmabadi Do the azp thing please. This is the third time in a row when I have roslyn-CI job stuck at "Waiting for status to be reported". It seems something is broken there

And also enable auto-merge please

@CyrusNajmabadi
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@CyrusNajmabadi
Copy link
Member

Thanks!

auto-merge was automatically disabled February 22, 2023 11:30

Head branch was pushed to by a user without write access

@DoctorKrolic
Copy link
Contributor Author

@CyrusNajmabadi Updated test that previously checked for no completions after ... Please verify that the change is correct

@CyrusNajmabadi
Copy link
Member

Thanks!

@DoctorKrolic
Copy link
Contributor Author

@CyrusNajmabadi The result of not treating .. as a trigger sequence is a break in list pattern completions (completions are no longer invoked in cases like typing a dot in someCollection is [.$$] scenario). I looked through code and it seems that at that stage we don't have a syntax tree to check that, only source text and position. What should we do?

@CyrusNajmabadi
Copy link
Member

(completions are no longer invoked in cases like typing a dot in someCollection is [.$$] scenario)

Completion should not be triggered here on dot. dot completion is for actually dotting off of some entity. There is no entity here, so i would not expect the second dot to trigger anything.

@DoctorKrolic
Copy link
Contributor Author

Well, failed test verify opposite, e.g.:

<WpfTheory, CombinatorialData>
Public Async Function CompletionInSlicePattern_NullKeyword(showCompletionInArgumentLists As Boolean) As Task
Using state = TestStateFactory.CreateCSharpTestState(
<Document>
public class RestType
{
}
public class C
{
public int Length => 0;
public int this[int i] => 0;
public RestType Slice(int i, int j) => null;
void M(C c)
{
_ = c is [ 0, $$ ]
}
}
</Document>,
showCompletionInArgumentLists:=showCompletionInArgumentLists, languageVersion:=LanguageVersion.Preview)
state.SendTypeChars("..")
Await state.AssertSelectedCompletionItem(displayText:="async", isHardSelected:=False)
state.SendTypeChars("nu")
Await state.AssertSelectedCompletionItem(displayText:="null", isHardSelected:=True)
state.SendTab()
Await state.AssertNoCompletionSession()
Assert.Contains("c is [ 0, ..null ]", state.GetLineTextFromCaretPosition(), StringComparison.Ordinal)
End Using
End Function

Should I modify all such tests to invoke completion list explicitly?

@CyrusNajmabadi
Copy link
Member

Should I modify all such tests to invoke completion list explicitly?

No. Update the test to show hte new behavior, and then have a new test that tests explicit-invoke. Thanks :)

auto-merge was automatically disabled February 23, 2023 03:08

Head branch was pushed to by a user without write access

@DoctorKrolic
Copy link
Contributor Author

I've updated tests to verify both behaviours at the same time. We just verify that after typing the there is no completion session, then eplicitely invoke the list and verify everything else. This makes us avoid a lot of code duplication

@DoctorKrolic
Copy link
Contributor Author

@CyrusNajmabadi PTAL

@CyrusNajmabadi
Copy link
Member

Thanks!

@CyrusNajmabadi CyrusNajmabadi merged commit 3aa2c51 into dotnet:main Feb 24, 2023
@ghost ghost added this to the Next milestone Feb 24, 2023
@DoctorKrolic DoctorKrolic deleted the completions-in-range-expression branch February 24, 2023 21:03
@RikkiGibson RikkiGibson modified the milestones: Next, 17.6 P2 Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Completions are missing at the end of a range operator
3 participants