Skip to content

Completion: Conditional access on nullable parameter suggests Nullable members#54491

Merged
CyrusNajmabadi merged 6 commits intodotnet:mainfrom
MaStr11:CompletionUnrwapNullableOnParameters
Jul 1, 2021
Merged

Completion: Conditional access on nullable parameter suggests Nullable members#54491
CyrusNajmabadi merged 6 commits intodotnet:mainfrom
MaStr11:CompletionUnrwapNullableOnParameters

Conversation

@MaStr11
Copy link
Copy Markdown
Contributor

@MaStr11 MaStr11 commented Jun 30, 2021

Fix #54361

Root cause is here:

if (symbol is IParameterSymbol parameter)
{
if (parameter.IsThis && expression.IsInStaticContext())
return default;
containerSymbol = symbol;

The containerSymbol was fixed before with RemoveNullableIfPresent, but the parameter symbols type is not. This PR fixes this in GetMemberSymbolsForParameter.

@MaStr11 MaStr11 requested a review from a team as a code owner June 30, 2021 10:00
@ghost ghost added the Area-IDE label Jun 30, 2021
{
void M(System.DateTime? dt)
{
dt?.$$
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider adding another test without the normal member access (dt.$$)

Copy link
Copy Markdown
Contributor Author

@MaStr11 MaStr11 Jun 30, 2021

Choose a reason for hiding this comment

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

You are right. Due to the unconditionally RemoveNullableIfPresent, dt.$$ suggest DateTime members, which is wrong. Good catch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Test added in c1bb0ca and fix was done in f0bab57

}
";
await VerifyItemExistsAsync(markup, "Day");
await VerifyItemIsAbsentAsync(markup, "value");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Typo? Should this have been Value?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

CopyPaste from the test above. Will fix it.

Copy link
Copy Markdown
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

Consider adding a test for VB as well.

@MaStr11
Copy link
Copy Markdown
Contributor Author

MaStr11 commented Jul 1, 2021

Consider adding a test for VB as well.

I added tests for VB (61987a6) and this revealed a broken behavior in VB. This was fixed in b325301.

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge July 1, 2021 17:32
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

thanks!

@CyrusNajmabadi CyrusNajmabadi merged commit a88ed4b into dotnet:main Jul 1, 2021
@ghost ghost added this to the Next milestone Jul 1, 2021
@jinujoseph jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jul 5, 2021
@allisonchou allisonchou modified the milestones: Next, 17.0.P3 Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE 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.

the IntelliSense Type error

5 participants