-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Offer to add parameter to constructor with no existing parameters #33624
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -642,5 +642,46 @@ public C(int i, int hello) | |
}" | ||
); | ||
} | ||
|
||
[WorkItem(33602, "https://github.com/dotnet/roslyn/issues/33602")] | ||
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddConstructorParametersFromMembers)] | ||
public async Task TestConstructorWithNoParameters() | ||
{ | ||
await TestInRegularAndScriptAsync( | ||
@" | ||
class C | ||
{ | ||
[|int i; | ||
int Hello { get; set; }|] | ||
public C() | ||
{ | ||
} | ||
}", | ||
@" | ||
class C | ||
{ | ||
int i; | ||
int Hello { get; set; } | ||
public C(int i, int hello) | ||
{ | ||
this.i = i; | ||
Hello = hello; | ||
} | ||
}" | ||
); | ||
} | ||
|
||
[WorkItem(33602, "https://github.com/dotnet/roslyn/issues/33602")] | ||
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddConstructorParametersFromMembers)] | ||
public async Task TestDefaultConstructor() | ||
{ | ||
await TestMissingAsync( | ||
@" | ||
class C | ||
{ | ||
[|int i;|] | ||
int Hello { get; set; } | ||
}"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably shoudl offer to create an explicit constructor here since hte implicit one def exists. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The "generate constructor" refactoring is offered here, and it will generate the constructor with the selected field as a parameter. The "add parameters to constructor" is not offered, but I think that is expected. |
||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,28 +77,31 @@ private bool TryInitialize( | |
/// <summary> | ||
/// Try to find a constructor in <paramref name="containingType"/> whose parameters is the subset of <paramref name="parameters"/> by comparing name. | ||
/// If multiple constructors meet the condition, the one with more parameters will be returned. | ||
/// It will not consider those constructors as potential candidates if: | ||
/// 1. Constructor with empty parameter list. | ||
/// 2. Constructor's parameter list contains 'ref' or 'params' | ||
/// It will not consider those constructors as potential candidates if the constructor's parameter list | ||
/// contains 'ref' or 'params' | ||
/// </summary> | ||
private IMethodSymbol GetDelegatedConstructorBasedOnParameterNames( | ||
INamedTypeSymbol containingType, | ||
ImmutableArray<IParameterSymbol> parameters) | ||
{ | ||
var parameterNames = parameters.SelectAsArray(p => p.Name); | ||
return containingType.InstanceConstructors | ||
.Where(constructor => AreParametersContainedInConstructor(constructor, parameterNames)) | ||
.Where(constructor => IsApplicableConstructor(constructor, parameterNames)) | ||
.OrderByDescending(constructor => constructor.Parameters.Length) | ||
.FirstOrDefault(); | ||
} | ||
|
||
private bool AreParametersContainedInConstructor( | ||
private bool IsApplicableConstructor( | ||
IMethodSymbol constructor, | ||
ImmutableArray<string> parametersName) | ||
{ | ||
var constructorParams = constructor.Parameters; | ||
return constructorParams.Length > 0 | ||
&& constructorParams.All(parameter => parameter.RefKind == RefKind.None) | ||
if (constructorParams.Length == 0) | ||
{ | ||
return !constructor.IsImplicitlyDeclared; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the behavior if we only have an implicit default ctor? Refactoring will not be available? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, i would prefer it's avialable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See this: #33624 (comment) |
||
} | ||
|
||
return constructorParams.All(parameter => parameter.RefKind == RefKind.None) | ||
&& !constructorParams.Any(p => p.IsParams) | ||
&& parametersName.Except(constructorParams.Select(p => p.Name)).Any(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also have a test with only implicit default ctor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we have this test: