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.

Sign up for GitHub

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

Jump to bottom

Treat record positional parameters as properties #48329

Merged
merged 6 commits into from
Dec 14, 2020
Merged

Treat record positional parameters as properties #48329

merged 6 commits into from
Dec 14, 2020

Conversation

Youssef1313
Copy link
Member

Fixes #48310

@Youssef1313 Youssef1313 requested a review from a team as a code owner October 5, 2020 16:11
@jinujoseph jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Oct 5, 2020
davidwengier
davidwengier reviewed Oct 5, 2020
View reviewed changes
Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

May be a nit, but in my opinion it would be nicer to add an IsPrimaryConstructorParameter method to handle this case, and save having to add a parameter to the lambda in this way. It would also make the resulting code self documenting.

@Youssef1313
Copy link
Member Author

@davidwengier I think I'd still need the lambda parameter if I will rely on IsLastTokenOfType. But I might include a new parameter for IsLastTokenOfType that will hold Func<TSyntaxNode, bool>. This argument will always be _ => true except for the new introduced method IsPrimaryConstructorParameter. This is the approach that comes to mind if I introduced the method you suggested. Is there a better approach?

@davidwengier
Copy link
Contributor

davidwengier commented Oct 7, 2020 •
edited
Loading

@Youssef1313 I haven't thought it through completely, so if you think its not possible, or any better, that's cool, I just pictured something like the tuple handling (see IsTupleTypeElement), where you can use token.GetAncestor<TSyntaxNode>(), and if the node is a parameter of a primary constructor, then go from there.

@Youssef1313
Copy link
Member Author

@davidwengier I made it as a separate method as you suggested and relied on calling token.GetAncestor. The only problem with this approach is that GetAncestor is now called twice for the same token. One time in IsLastTokenOfType and the other in IsPrimaryConstructorParameter. I assume the call is cheap and won't impact performance.

Youssef1313
Youssef1313 commented Oct 7, 2020
View reviewed changes
...Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.DeclarationInfo.cs
private static bool IsPrimaryConstructorParameter(SyntaxToken token, SemanticModel semanticModel,
CancellationToken cancellationToken, out NameDeclarationInfo result)
{
result = IsLastTokenOfType<ParameterSyntax>(
Copy link
Member Author

Choose a reason for hiding this comment

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

By the way, the method name here is very confusing. The name suggests that it returns a bool, but it doesn't.

I would suggest an analyzer to warn for things like this.
Would this be an IDExxxx rule or a CAxxxx rule?

@Youssef1313 Youssef1313 force-pushed the suggestion-for-record-param branch from 347d428 to b5732e9 Compare October 7, 2020 14:12
davidwengier
davidwengier reviewed Oct 8, 2020
View reviewed changes
...Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.DeclarationInfo.cs Outdated
cancellationToken);

return result.Type != null &&
token.GetAncestor<ParameterSyntax>().Parent.IsParentKind(SyntaxKind.RecordDeclaration);
Copy link
Contributor

Choose a reason for hiding this comment

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

This check should come first, and result should be default if it fails.

@runfoapp runfoapp bot mentioned this pull request Oct 16, 2020
Closed
@Youssef1313
Copy link
Member Author

Closing/re-opening for fresh build.

@Youssef1313 Youssef1313 closed this Nov 4, 2020
@Youssef1313 Youssef1313 deleted the suggestion-for-record-param branch November 4, 2020 13:31
@Youssef1313 Youssef1313 restored the suggestion-for-record-param branch November 4, 2020 13:31
@Youssef1313 Youssef1313 reopened this Nov 4, 2020
@Youssef1313 Youssef1313 reopened this Dec 10, 2020
@Youssef1313
Copy link
Member Author

@davidwengier for reviewing after applying review feedback

davidwengier
davidwengier approved these changes Dec 14, 2020
View reviewed changes
Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

Thanks!

@davidwengier davidwengier merged commit 3655d54 into dotnet:master Dec 14, 2020
@ghost ghost added this to the Next milestone Dec 14, 2020
@Youssef1313 Youssef1313 deleted the suggestion-for-record-param branch December 14, 2020 06:17
333fred added a commit to 333fred/roslyn that referenced this pull request Dec 15, 2020
@333fred
Merge remote-tracking branch 'upstream/master' into merge-master
b378406 
* upstream/master: (241 commits)
  Allow pattern matching `null` against pointer types when the pointer types contain nested type parameters (dotnet#49915)
  Remove document extension method and convert usages to use the text buffer extension method.
  VB: Strengthen implementation of `PropertySymbol.IsWritable` against NullReferenceException (dotnet#49962)
  Add switch to skip nullable analysis (dotnet#49876)
  Update dependencies from https://github.com/dotnet/roslyn build 20201211.16 (dotnet#49958)
  Treat record positional parameters as properties (dotnet#48329)
  [master] Update dependencies from dotnet/roslyn (dotnet#49395)
  VB: Ensure array access indexes undergo conversion to integer even when there is a mismatch with array rank. (dotnet#49907)
  Disable OOP when running as cloud environment client VS instance
  Rename workspace context method (and unify impls) to better represent the condition being checked
  Report non-Const locals used in an expression that must have a constant value. (dotnet#49912)
  Add support for more ServiceAudience values (dotnet#49914)
  Handle ref-containing structs returned by value from function-pointers (dotnet#49883)
  Fix error on out param of extern local function (dotnet#49860)
  Fix constructor exit warnings for generic NotNull (dotnet#49841)
  Loc updates
  Prefer more specific path map key (dotnet#49670)
  Rename `_availablelocalFunctionOrdinal` to `_availableLocalFunctionOrdinal` (dotnet#49901)
  Fix namespace so that external access wrapper type can be accessed from UT.
  XamlProjectService fixes (dotnet#49711)
  ...
@dibarbet dibarbet modified the milestones: Next, 16.9.P3 Dec 19, 2020
@mikadumont mikadumont mentioned this pull request Jan 25, 2021
Closed
@Youssef1313 Youssef1313 mentioned this pull request Feb 18, 2021
Closed
92 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwengier davidwengier davidwengier approved these changes

Assignees

@sharwell sharwell

Labels
Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Milestone
16.9.P3
Development

Successfully merging this pull request may close these issues.

Suggested positional parameter name should be pascal-cased
5 participants
@Youssef1313 @davidwengier @sharwell @dibarbet @jinujoseph